* Re: [patch] isdn: fix a wrapping bug in isdn_ppp_ioctl()
@ 2012-10-10 10:19 ` Joe Perches
0 siblings, 0 replies; 30+ messages in thread
From: Joe Perches @ 2012-10-10 10:19 UTC (permalink / raw)
To: Dan Carpenter
Cc: Karsten Keil, David S. Miller, Masanari Iida, netdev,
kernel-janitors
On Wed, 2012-10-10 at 12:42 +0300, Dan Carpenter wrote:
> "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 <dan.carpenter@oracle.com>
>
> diff --git a/drivers/isdn/i4l/isdn_ppp.c b/drivers/isdn/i4l/isdn_ppp.c
> index a1e7601..69b5b58 100644
> --- a/drivers/isdn/i4l/isdn_ppp.c
> +++ b/drivers/isdn/i4l/isdn_ppp.c
> @@ -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(-)
diff --git a/drivers/isdn/i4l/isdn_ppp.c b/drivers/isdn/i4l/isdn_ppp.c
index a1e7601..6438d0c 100644
--- a/drivers/isdn/i4l/isdn_ppp.c
+++ b/drivers/isdn/i4l/isdn_ppp.c
@@ -471,7 +471,7 @@ int
isdn_ppp_ioctl(int min, struct file *file, unsigned int cmd, unsigned long arg)
{
unsigned long val;
- int r, i, j;
+ int r;
struct ippp_struct *is;
isdn_net_local *lp;
struct isdn_ppp_comp_data data;
@@ -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))))
return r;
}
break;
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [patch] isdn: fix a wrapping bug in isdn_ppp_ioctl()
2012-10-10 10:19 ` Joe Perches
@ 2012-10-10 10:41 ` Joe Perches
-1 siblings, 0 replies; 30+ messages in thread
From: Joe Perches @ 2012-10-10 10:41 UTC (permalink / raw)
To: Dan Carpenter
Cc: Karsten Keil, David S. Miller, Masanari Iida, netdev,
kernel-janitors
On Wed, 2012-10-10 at 03:19 -0700, Joe Perches wrote:
> diff --git a/drivers/isdn/i4l/isdn_ppp.c b/drivers/isdn/i4l/isdn_ppp.c
[]
> struct isdn_ppp_comp_data data;
> @@ -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, };
s/BITS_PER_LONG/sizeof(long)/duh...
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [patch] isdn: fix a wrapping bug in isdn_ppp_ioctl()
@ 2012-10-10 10:41 ` Joe Perches
0 siblings, 0 replies; 30+ messages in thread
From: Joe Perches @ 2012-10-10 10:41 UTC (permalink / raw)
To: Dan Carpenter
Cc: Karsten Keil, David S. Miller, Masanari Iida, netdev,
kernel-janitors
On Wed, 2012-10-10 at 03:19 -0700, Joe Perches wrote:
> diff --git a/drivers/isdn/i4l/isdn_ppp.c b/drivers/isdn/i4l/isdn_ppp.c
[]
> struct isdn_ppp_comp_data data;
> @@ -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, };
s/BITS_PER_LONG/sizeof(long)/duh...
^ permalink raw reply [flat|nested] 30+ messages in thread
* RE: [patch] isdn: fix a wrapping bug in isdn_ppp_ioctl()
2012-10-10 10:19 ` Joe Perches
@ 2012-10-10 10:42 ` David Laight
-1 siblings, 0 replies; 30+ messages in thread
From: David Laight @ 2012-10-10 10:42 UTC (permalink / raw)
To: Joe Perches, Dan Carpenter
Cc: Karsten Keil, David S. Miller, Masanari Iida, netdev,
kernel-janitors
> - unsigned long protos[8] = {0,};
> + DECLARE_BITMAP(protos, BITS_PER_LONG * 8) = { 0, };
...
> - 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
^ permalink raw reply [flat|nested] 30+ messages in thread* RE: [patch] isdn: fix a wrapping bug in isdn_ppp_ioctl()
@ 2012-10-10 10:42 ` David Laight
0 siblings, 0 replies; 30+ messages in thread
From: David Laight @ 2012-10-10 10:42 UTC (permalink / raw)
To: Joe Perches, Dan Carpenter
Cc: Karsten Keil, David S. Miller, Masanari Iida, netdev,
kernel-janitors
> - unsigned long protos[8] = {0,};
> + DECLARE_BITMAP(protos, BITS_PER_LONG * 8) = { 0, };
...
> - 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
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [patch] isdn: fix a wrapping bug in isdn_ppp_ioctl()
2012-10-10 10:42 ` David Laight
@ 2012-10-10 11:15 ` Joe Perches
-1 siblings, 0 replies; 30+ messages in thread
From: Joe Perches @ 2012-10-10 11:15 UTC (permalink / raw)
To: David Laight
Cc: Dan Carpenter, Karsten Keil, David S. Miller, Masanari Iida,
netdev, kernel-janitors
On Wed, 2012-10-10 at 11:42 +0100, David Laight wrote:
> > - unsigned long protos[8] = {0,};
> > + DECLARE_BITMAP(protos, BITS_PER_LONG * 8) = { 0, };
> ...
> > - 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
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [patch] isdn: fix a wrapping bug in isdn_ppp_ioctl()
@ 2012-10-10 11:15 ` Joe Perches
0 siblings, 0 replies; 30+ messages in thread
From: Joe Perches @ 2012-10-10 11:15 UTC (permalink / raw)
To: David Laight
Cc: Dan Carpenter, Karsten Keil, David S. Miller, Masanari Iida,
netdev, kernel-janitors
On Wed, 2012-10-10 at 11:42 +0100, David Laight wrote:
> > - unsigned long protos[8] = {0,};
> > + DECLARE_BITMAP(protos, BITS_PER_LONG * 8) = { 0, };
> ...
> > - 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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [patch] isdn: fix a wrapping bug in isdn_ppp_ioctl()
2012-10-10 10:19 ` Joe Perches
@ 2012-10-10 12:05 ` Andreas Schwab
-1 siblings, 0 replies; 30+ messages in thread
From: Andreas Schwab @ 2012-10-10 12:05 UTC (permalink / raw)
To: Joe Perches
Cc: Dan Carpenter, Karsten Keil, David S. Miller, Masanari Iida,
netdev, kernel-janitors
Joe Perches <joe@perches.com> writes:
> @@ -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."
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [patch] isdn: fix a wrapping bug in isdn_ppp_ioctl()
@ 2012-10-10 12:05 ` Andreas Schwab
0 siblings, 0 replies; 30+ messages in thread
From: Andreas Schwab @ 2012-10-10 12:05 UTC (permalink / raw)
To: Joe Perches
Cc: Dan Carpenter, Karsten Keil, David S. Miller, Masanari Iida,
netdev, kernel-janitors
Joe Perches <joe@perches.com> writes:
> @@ -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."
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [patch] isdn: fix a wrapping bug in isdn_ppp_ioctl()
2012-10-10 12:05 ` Andreas Schwab
@ 2012-10-10 13:00 ` Joe Perches
-1 siblings, 0 replies; 30+ messages in thread
From: Joe Perches @ 2012-10-10 13:00 UTC (permalink / raw)
To: Andreas Schwab
Cc: Dan Carpenter, Karsten Keil, David S. Miller, Masanari Iida,
netdev, kernel-janitors
On Wed, 2012-10-10 at 14:05 +0200, Andreas Schwab wrote:
> Joe Perches <joe@perches.com> writes:
>
> > @@ -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.
How does it do that?
include/linux/bitops.h:#define BIT_MASK(nr) (1UL << ((nr) % BITS_PER_LONG))
include/linux/bitops.h:#define BIT_WORD(nr) ((nr) / BITS_PER_LONG)
include/asm-generic/bitops/atomic.h:static inline void set_bit(int nr, volatile unsigned long *addr)
include/asm-generic/bitops/atomic.h-{
include/asm-generic/bitops/atomic.h- unsigned long mask = BIT_MASK(nr);
include/asm-generic/bitops/atomic.h- unsigned long *p = ((unsigned long *)addr) + BIT_WORD(nr);
include/asm-generic/bitops/atomic.h- unsigned long flags;
include/asm-generic/bitops/atomic.h-
include/asm-generic/bitops/atomic.h- _atomic_spin_lock_irqsave(p, flags);
include/asm-generic/bitops/atomic.h- *p |= mask;
include/asm-generic/bitops/atomic.h- _atomic_spin_unlock_irqrestore(p, flags);
include/asm-generic/bitops/atomic.h-}
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [patch] isdn: fix a wrapping bug in isdn_ppp_ioctl()
@ 2012-10-10 13:00 ` Joe Perches
0 siblings, 0 replies; 30+ messages in thread
From: Joe Perches @ 2012-10-10 13:00 UTC (permalink / raw)
To: Andreas Schwab
Cc: Dan Carpenter, Karsten Keil, David S. Miller, Masanari Iida,
netdev, kernel-janitors
On Wed, 2012-10-10 at 14:05 +0200, Andreas Schwab wrote:
> Joe Perches <joe@perches.com> writes:
>
> > @@ -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.
How does it do that?
include/linux/bitops.h:#define BIT_MASK(nr) (1UL << ((nr) % BITS_PER_LONG))
include/linux/bitops.h:#define BIT_WORD(nr) ((nr) / BITS_PER_LONG)
include/asm-generic/bitops/atomic.h:static inline void set_bit(int nr, volatile unsigned long *addr)
include/asm-generic/bitops/atomic.h-{
include/asm-generic/bitops/atomic.h- unsigned long mask = BIT_MASK(nr);
include/asm-generic/bitops/atomic.h- unsigned long *p = ((unsigned long *)addr) + BIT_WORD(nr);
include/asm-generic/bitops/atomic.h- unsigned long flags;
include/asm-generic/bitops/atomic.h-
include/asm-generic/bitops/atomic.h- _atomic_spin_lock_irqsave(p, flags);
include/asm-generic/bitops/atomic.h- *p |= mask;
include/asm-generic/bitops/atomic.h- _atomic_spin_unlock_irqrestore(p, flags);
include/asm-generic/bitops/atomic.h-}
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [patch] isdn: fix a wrapping bug in isdn_ppp_ioctl()
2012-10-10 13:00 ` Joe Perches
@ 2012-10-10 13:58 ` Andreas Schwab
-1 siblings, 0 replies; 30+ messages in thread
From: Andreas Schwab @ 2012-10-10 13:58 UTC (permalink / raw)
To: Joe Perches
Cc: Dan Carpenter, Karsten Keil, David S. Miller, Masanari Iida,
netdev, kernel-janitors
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."
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [patch] isdn: fix a wrapping bug in isdn_ppp_ioctl()
@ 2012-10-10 13:58 ` Andreas Schwab
0 siblings, 0 replies; 30+ messages in thread
From: Andreas Schwab @ 2012-10-10 13:58 UTC (permalink / raw)
To: Joe Perches
Cc: Dan Carpenter, Karsten Keil, David S. Miller, Masanari Iida,
netdev, kernel-janitors
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."
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [patch] isdn: fix a wrapping bug in isdn_ppp_ioctl()
2012-10-10 13:58 ` Andreas Schwab
@ 2012-10-10 14:41 ` Joe Perches
-1 siblings, 0 replies; 30+ messages in thread
From: Joe Perches @ 2012-10-10 14:41 UTC (permalink / raw)
To: Andreas Schwab
Cc: Dan Carpenter, Karsten Keil, David S. Miller, Masanari Iida,
netdev, kernel-janitors
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.
> > - if ((r = set_arg(argp, protos, 8 * sizeof(long))))
set_arg's 2nd arg is bytes not bits.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [patch] isdn: fix a wrapping bug in isdn_ppp_ioctl()
@ 2012-10-10 14:41 ` Joe Perches
0 siblings, 0 replies; 30+ messages in thread
From: Joe Perches @ 2012-10-10 14:41 UTC (permalink / raw)
To: Andreas Schwab
Cc: Dan Carpenter, Karsten Keil, David S. Miller, Masanari Iida,
netdev, kernel-janitors
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.
> > - if ((r = set_arg(argp, protos, 8 * sizeof(long))))
set_arg's 2nd arg is bytes not bits.
^ permalink raw reply [flat|nested] 30+ messages in thread
* RE: [patch] isdn: fix a wrapping bug in isdn_ppp_ioctl()
2012-10-10 14:41 ` Joe Perches
@ 2012-10-10 14:59 ` David Laight
-1 siblings, 0 replies; 30+ messages in thread
From: David Laight @ 2012-10-10 14:59 UTC (permalink / raw)
To: Joe Perches, Andreas Schwab
Cc: Dan Carpenter, Karsten Keil, David S. Miller, Masanari Iida,
netdev, kernel-janitors
> -----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:
> > 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.
>
> > > - 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
^ permalink raw reply [flat|nested] 30+ messages in thread
* RE: [patch] isdn: fix a wrapping bug in isdn_ppp_ioctl()
@ 2012-10-10 14:59 ` David Laight
0 siblings, 0 replies; 30+ messages in thread
From: David Laight @ 2012-10-10 14:59 UTC (permalink / raw)
To: Joe Perches, Andreas Schwab
Cc: Dan Carpenter, Karsten Keil, David S. Miller, Masanari Iida,
netdev, kernel-janitors
> -----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:
> > 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.
>
> > > - 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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [patch] isdn: fix a wrapping bug in isdn_ppp_ioctl()
2012-10-10 14:59 ` David Laight
@ 2012-10-10 15:19 ` Joe Perches
-1 siblings, 0 replies; 30+ messages in thread
From: Joe Perches @ 2012-10-10 15:19 UTC (permalink / raw)
To: David Laight
Cc: Andreas Schwab, Dan Carpenter, Karsten Keil, David S. Miller,
Masanari Iida, netdev, kernel-janitors
On Wed, 2012-10-10 at 15:59 +0100, David Laight wrote:
> 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])
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [patch] isdn: fix a wrapping bug in isdn_ppp_ioctl()
@ 2012-10-10 15:19 ` Joe Perches
0 siblings, 0 replies; 30+ messages in thread
From: Joe Perches @ 2012-10-10 15:19 UTC (permalink / raw)
To: David Laight
Cc: Andreas Schwab, Dan Carpenter, Karsten Keil, David S. Miller,
Masanari Iida, netdev, kernel-janitors
On Wed, 2012-10-10 at 15:59 +0100, David Laight wrote:
> 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])
^ permalink raw reply [flat|nested] 30+ messages in thread* RE: [patch] isdn: fix a wrapping bug in isdn_ppp_ioctl()
2012-10-10 15:19 ` Joe Perches
@ 2012-10-10 15:44 ` David Laight
-1 siblings, 0 replies; 30+ messages in thread
From: David Laight @ 2012-10-10 15:44 UTC (permalink / raw)
To: Joe Perches
Cc: Andreas Schwab, Dan Carpenter, Karsten Keil, David S. Miller,
Masanari Iida, netdev, kernel-janitors
> On Wed, 2012-10-10 at 15:59 +0100, David Laight wrote:
> > 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
^ permalink raw reply [flat|nested] 30+ messages in thread* RE: [patch] isdn: fix a wrapping bug in isdn_ppp_ioctl()
@ 2012-10-10 15:44 ` David Laight
0 siblings, 0 replies; 30+ messages in thread
From: David Laight @ 2012-10-10 15:44 UTC (permalink / raw)
To: Joe Perches
Cc: Andreas Schwab, Dan Carpenter, Karsten Keil, David S. Miller,
Masanari Iida, netdev, kernel-janitors
> On Wed, 2012-10-10 at 15:59 +0100, David Laight wrote:
> > 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
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [patch] isdn: fix a wrapping bug in isdn_ppp_ioctl()
2012-10-10 15:44 ` David Laight
@ 2012-10-10 15:52 ` Joe Perches
-1 siblings, 0 replies; 30+ messages in thread
From: Joe Perches @ 2012-10-10 15:52 UTC (permalink / raw)
To: David Laight
Cc: Andreas Schwab, Dan Carpenter, Karsten Keil, David S. Miller,
Masanari Iida, netdev, kernel-janitors
On Wed, 2012-10-10 at 16:44 +0100, David Laight wrote:
> > On Wed, 2012-10-10 at 15:59 +0100, David Laight wrote:
> > > 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?
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [patch] isdn: fix a wrapping bug in isdn_ppp_ioctl()
@ 2012-10-10 15:52 ` Joe Perches
0 siblings, 0 replies; 30+ messages in thread
From: Joe Perches @ 2012-10-10 15:52 UTC (permalink / raw)
To: David Laight
Cc: Andreas Schwab, Dan Carpenter, Karsten Keil, David S. Miller,
Masanari Iida, netdev, kernel-janitors
On Wed, 2012-10-10 at 16:44 +0100, David Laight wrote:
> > On Wed, 2012-10-10 at 15:59 +0100, David Laight wrote:
> > > 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?
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [patch] isdn: fix a wrapping bug in isdn_ppp_ioctl()
2012-10-10 15:52 ` Joe Perches
@ 2012-10-12 7:49 ` Karsten Keil
-1 siblings, 0 replies; 30+ messages in thread
From: Karsten Keil @ 2012-10-12 7:49 UTC (permalink / raw)
To: Joe Perches
Cc: David Laight, Andreas Schwab, Dan Carpenter, Karsten Keil,
David S. Miller, Masanari Iida, netdev, kernel-janitors
Hi,
Am 10.10.2012 17:52, schrieb Joe Perches:
> On Wed, 2012-10-10 at 16:44 +0100, David Laight wrote:
>>> On Wed, 2012-10-10 at 15:59 +0100, David Laight wrote:
>>>> 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?
>
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
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [patch] isdn: fix a wrapping bug in isdn_ppp_ioctl()
@ 2012-10-12 7:49 ` Karsten Keil
0 siblings, 0 replies; 30+ messages in thread
From: Karsten Keil @ 2012-10-12 7:49 UTC (permalink / raw)
To: Joe Perches
Cc: David Laight, Andreas Schwab, Dan Carpenter, Karsten Keil,
David S. Miller, Masanari Iida, netdev, kernel-janitors
Hi,
Am 10.10.2012 17:52, schrieb Joe Perches:
> On Wed, 2012-10-10 at 16:44 +0100, David Laight wrote:
>>> On Wed, 2012-10-10 at 15:59 +0100, David Laight wrote:
>>>> 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?
>
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
^ permalink raw reply [flat|nested] 30+ messages in thread