Patchwork isdn: fix a wrapping bug in isdn_ppp_ioctl()

login
register
mail settings
Submitter Joe Perches
Date Oct. 10, 2012, 10:19 a.m.
Message ID <1349864358.2386.27.camel@joe-AO722>
Download mbox | patch
Permalink /patch/190606/
State RFC
Delegated to: David Miller
Headers show

Comments

Joe Perches - Oct. 10, 2012, 10:19 a.m.
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(-)



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Joe Perches - Oct. 10, 2012, 10:41 a.m.
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...


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Laight - Oct. 10, 2012, 10:42 a.m.
> -		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



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Joe Perches - Oct. 10, 2012, 11:15 a.m.
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


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andreas Schwab - Oct. 10, 2012, 12:05 p.m.
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.
Joe Perches - Oct. 10, 2012, 1 p.m.
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-}


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andreas Schwab - Oct. 10, 2012, 1:58 p.m.
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.
Joe Perches - Oct. 10, 2012, 2:41 p.m.
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.


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Laight - Oct. 10, 2012, 2:59 p.m.
> -----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



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Joe Perches - Oct. 10, 2012, 3:19 p.m.
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])


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Laight - Oct. 10, 2012, 3:44 p.m.
> 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


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Joe Perches - Oct. 10, 2012, 3:52 p.m.
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?

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Karsten Keil - Oct. 12, 2012, 7:49 a.m.
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

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

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;