From: Dan Carpenter <hidden> Date: 2012-10-10 09:42:43
"protos" is an array of unsigned longs and "i" is the number of bits in
an unsigned long so we need to use 1UL as well to prevent the shift
from wrapping around.
Signed-off-by: Dan Carpenter <redacted>
From: Joe Perches <joe@perches.com> Date: 2012-10-10 10:19:19
On Wed, 2012-10-10 at 12:42 +0300, Dan Carpenter wrote:
quoted hunk
"protos" is an array of unsigned longs and "i" is the number of bits in
an unsigned long so we need to use 1UL as well to prevent the shift
from wrapping around.
Signed-off-by: Dan Carpenter <redacted>
@@ -595,7 +595,7 @@ isdn_ppp_ioctl(int min, struct file *file, unsigned int cmd, unsigned long arg)j=ipc->num/(sizeof(long)*8);i=ipc->num%(sizeof(long)*8);if(j<8)-protos[j]|=(0x1<<i);+protos[j]|=(1UL<<i);
This looks like a bitmap and probably it should use
DECLARE_BITMAP and set_bit.
Also, it looks like the set_arg size is wrong and is
a stack leak.
Maybe:
drivers/isdn/i4l/isdn_ppp.c | 13 ++++++-------
1 file changed, 6 insertions(+), 7 deletions(-)
- if ((r = set_arg(argp, protos, 8 * sizeof(long))))
+ if ((r = set_arg(argp, protos, sizeof(protos))))
That change makes a big assumption about the implementation
of DECLARE_BITMAP().
Unless it is guaranteed to be implemented as 'unsigned long[]'
then you've changed what the code might do.
David
- if ((r = set_arg(argp, protos, 8 * sizeof(long))))
+ if ((r = set_arg(argp, protos, sizeof(protos))))
That change makes a big assumption about the implementation
of DECLARE_BITMAP().
Unless it is guaranteed to be implemented as 'unsigned long[]'
then you've changed what the code might do.
Possible, but it's hard to imagine it changing.
The = { 0, } should probably be bitmap_zero
From: Andreas Schwab <hidden> Date: 2012-10-10 12:05:06
Joe Perches [off-list ref] writes:
quoted hunk
@@ -589,16 +589,15 @@ isdn_ppp_ioctl(int min, struct file *file, unsigned int cmd, unsigned long arg) break; case PPPIOCGCOMPRESSORS: {- unsigned long protos[8] = {0,};+ DECLARE_BITMAP(protos, BITS_PER_LONG * 8) = { 0, }; struct isdn_ppp_compressor *ipc = ipc_head;+ while (ipc) {- j = ipc->num / (sizeof(long) * 8);- i = ipc->num % (sizeof(long) * 8);- if (j < 8)- protos[j] |= (0x1 << i);+ if (ipc->num < BITS_PER_LONG * 8)+ set_bit(ipc->num, protos); ipc = ipc->next; }- if ((r = set_arg(argp, protos, 8 * sizeof(long))))+ if ((r = set_arg(argp, protos, sizeof(protos))))
This changes the bit endianess. Since protos is exported to user space
it is an ABI change.
Andreas.
--
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5
"And now for something completely different."
From: Andreas Schwab <hidden> Date: 2012-10-10 13:58:41
Sorry, I was misremembering the history of the bit ops. There has
historically been issues with varying bit order, but noadays set_bit is
always defined consistently with C shifts.
But another issue, set_bit is an atomic operation which may be
significantly more expensive than the equivalent C operation. Better
use __set_bit when atomicity is not needed.
Andreas.
--
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5
"And now for something completely different."
From: Joe Perches <joe@perches.com> Date: 2012-10-10 14:42:00
On Wed, 2012-10-10 at 15:58 +0200, Andreas Schwab wrote:
Sorry, I was misremembering the history of the bit ops. There has
historically been issues with varying bit order, but noadays set_bit is
always defined consistently with C shifts.
No worries. Anyway, the change was suggested to aid
reader comprehension. If it doesn't (and it seems not)
then it's not worth it.
Anyway, there is still the open question of an overrun/info
leak.
quoted
- if ((r = set_arg(argp, protos, 8 * sizeof(long))))
From: David Laight <hidden> Date: 2012-10-10 15:09:40
-----Original Message-----
From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org] On Behalf Of Joe Perches
Sent: 10 October 2012 15:42
To: Andreas Schwab
Cc: Dan Carpenter; Karsten Keil; David S. Miller; Masanari Iida; netdev@vger.kernel.org; kernel-
janitors@vger.kernel.org
Subject: Re: [patch] isdn: fix a wrapping bug in isdn_ppp_ioctl()
On Wed, 2012-10-10 at 15:58 +0200, Andreas Schwab wrote:
quoted
Sorry, I was misremembering the history of the bit ops. There has
historically been issues with varying bit order, but noadays set_bit is
always defined consistently with C shifts.
No worries. Anyway, the change was suggested to aid
reader comprehension. If it doesn't (and it seems not)
then it's not worth it.
Anyway, there is still the open question of an overrun/info
leak.
quoted
quoted
- if ((r = set_arg(argp, protos, 8 * sizeof(long))))
set_arg's 2nd arg is bytes not bits.
Seems to me the code is expecting 256 bits of data, not any multiple of int,
long or anything else.
David
From: David Laight <hidden> Date: 2012-10-10 15:45:10
On Wed, 2012-10-10 at 15:59 +0100, David Laight wrote:
quoted
Seems to me the code is expecting 256 bits of data, not any multiple of int,
long or anything else.
include/linux/isdn_ppp.h:#define PPPIOCGCOMPRESSORS _IOR('t',134,unsigned long [8])
That doesn't mean the whole thing makes any sense on 64bit systems.
A whole load of historic code used 'long' to ensure 32bit.
Some of that might have crept into Linux sources.
Since I suspect there are a maximum of 256 bits on both 32 and 64bit
systems, I wouldn't like to guess exactly how any particular 64bit
application chooses to check the bitmap.
The ioctl constant may be wrong on 64 bit systems.
David
From: Joe Perches <joe@perches.com> Date: 2012-10-10 15:52:24
On Wed, 2012-10-10 at 16:44 +0100, David Laight wrote:
quoted
On Wed, 2012-10-10 at 15:59 +0100, David Laight wrote:
quoted
Seems to me the code is expecting 256 bits of data, not any multiple of int,
long or anything else.
include/linux/isdn_ppp.h:#define PPPIOCGCOMPRESSORS _IOR('t',134,unsigned long [8])
That doesn't mean the whole thing makes any sense on 64bit systems.
A whole load of historic code used 'long' to ensure 32bit.
Some of that might have crept into Linux sources.
Very true, but it's exported via copy_to_user.
Since I suspect there are a maximum of 256 bits on both 32 and 64bit
systems, I wouldn't like to guess exactly how any particular 64bit
application chooses to check the bitmap.
The ioctl constant may be wrong on 64 bit systems.
shrug. Not much to do about it now.
isdn isn't very active.
Karsten? What do you think?
From: David Miller <davem@davemloft.net> Date: 2012-10-11 02:46:16
From: Dan Carpenter <redacted>
Date: Wed, 10 Oct 2012 12:42:18 +0300
"protos" is an array of unsigned longs and "i" is the number of bits in
an unsigned long so we need to use 1UL as well to prevent the shift
from wrapping around.
Signed-off-by: Dan Carpenter <redacted>
On Wed, 2012-10-10 at 16:44 +0100, David Laight wrote:
quoted
quoted
On Wed, 2012-10-10 at 15:59 +0100, David Laight wrote:
quoted
Seems to me the code is expecting 256 bits of data, not any multiple of int,
long or anything else.
include/linux/isdn_ppp.h:#define PPPIOCGCOMPRESSORS _IOR('t',134,unsigned long [8])
That doesn't mean the whole thing makes any sense on 64bit systems.
A whole load of historic code used 'long' to ensure 32bit.
Some of that might have crept into Linux sources.
Very true, but it's exported via copy_to_user.
quoted
Since I suspect there are a maximum of 256 bits on both 32 and 64bit
systems, I wouldn't like to guess exactly how any particular 64bit
application chooses to check the bitmap.
The ioctl constant may be wrong on 64 bit systems.
shrug. Not much to do about it now.
isdn isn't very active.
Karsten? What do you think?
I use ipppd as testbench to test remote connections via different PPP
clients running on a 64 bit system without problems so far - but I did
not use any compressions for some years, so maybe this code was never
tested on 64 bit and at least not on mixed 32/64 bit systems.
If I will find some time, I will check if the compression works.
I did not wrote this part, so I cannot say how the code should work
correctly out of the box, I need to analyze this first by myself.
Karsten