[patch] isdn: fix a wrapping bug in isdn_ppp_ioctl()

[patch] isdn: fix a wrapping bug in isdn_ppp_ioctl()

[Dan Carpenter]

"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);
 			ipc = ipc->next;
 		}
 		if ((r = set_arg(argp, protos, 8 * sizeof(long))))

Re: [patch] isdn: fix a wrapping bug in isdn_ppp_ioctl()

[Joe Perches]

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;

Re: [patch] isdn: fix a wrapping bug in isdn_ppp_ioctl()

[Joe Perches]

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...

RE: [patch] isdn: fix a wrapping bug in isdn_ppp_ioctl()

[David Laight]

> -		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

Re: [patch] isdn: fix a wrapping bug in isdn_ppp_ioctl()

[Joe Perches]

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

Re: [patch] isdn: fix a wrapping bug in isdn_ppp_ioctl()

[Andreas Schwab]

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."

Re: [patch] isdn: fix a wrapping bug in isdn_ppp_ioctl()

[Joe Perches]

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-}

Re: [patch] isdn: fix a wrapping bug in isdn_ppp_ioctl()

[Andreas Schwab]

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."

Re: [patch] isdn: fix a wrapping bug in isdn_ppp_ioctl()

[Joe Perches]

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.

RE: [patch] isdn: fix a wrapping bug in isdn_ppp_ioctl()

[David Laight]

> -----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

Re: [patch] isdn: fix a wrapping bug in isdn_ppp_ioctl()

[Joe Perches]

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])

RE: [patch] isdn: fix a wrapping bug in isdn_ppp_ioctl()

[David Laight]

> 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

Re: [patch] isdn: fix a wrapping bug in isdn_ppp_ioctl()

[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?

Re: [patch] isdn: fix a wrapping bug in isdn_ppp_ioctl()

[David Miller]

From: Dan Carpenter <dan.carpenter@oracle.com>
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 <dan.carpenter@oracle.com>

I'll apply this, thanks.

Re: [patch] isdn: fix a wrapping bug in isdn_ppp_ioctl()

[Karsten Keil]

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