[PATCH] smsc95xx: fix suspend buffer overflow

[PATCH] smsc95xx: fix suspend buffer overflow

[Steve Glendinning]

This patch fixes a buffer overflow introduced by bbd9f9e, where
the filter_mask array is accessed beyond its bounds.

Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Steve Glendinning <steve.glendinning@shawell.net>
---
 drivers/net/usb/smsc95xx.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
index 79d495d..6cdc504 100644
--- a/drivers/net/usb/smsc95xx.c
+++ b/drivers/net/usb/smsc95xx.c
@@ -1281,7 +1281,7 @@ static int smsc95xx_suspend(struct usb_interface *intf, pm_message_t message)
 	}
 
 	if (pdata->wolopts & (WAKE_BCAST | WAKE_MCAST | WAKE_ARP | WAKE_UCAST)) {
-		u32 *filter_mask = kzalloc(32, GFP_KERNEL);
+		u32 *filter_mask = kzalloc(sizeof(u32) * 32, GFP_KERNEL);
 		u32 command[2];
 		u32 offset[2];
 		u32 crc[4];
-- 
1.7.10.4

Re: [PATCH] smsc95xx: fix suspend buffer overflow

[Bjørn Mork]

Steve Glendinning <steve.glendinning@shawell.net> writes:

> This patch fixes a buffer overflow introduced by bbd9f9e, where
> the filter_mask array is accessed beyond its bounds.
>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Steve Glendinning <steve.glendinning@shawell.net>
> ---
>  drivers/net/usb/smsc95xx.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
> index 79d495d..6cdc504 100644
> --- a/drivers/net/usb/smsc95xx.c
> +++ b/drivers/net/usb/smsc95xx.c
> @@ -1281,7 +1281,7 @@ static int smsc95xx_suspend(struct usb_interface *intf, pm_message_t message)
>  	}
>  
>  	if (pdata->wolopts & (WAKE_BCAST | WAKE_MCAST | WAKE_ARP | WAKE_UCAST)) {
> -		u32 *filter_mask = kzalloc(32, GFP_KERNEL);
> +		u32 *filter_mask = kzalloc(sizeof(u32) * 32, GFP_KERNEL);
>  		u32 command[2];
>  		u32 offset[2];
>  		u32 crc[4];

I wonder... all these magic constants (32, 2, 2, 4) obviously relate to
the maximum number of supported filters (8).  It would be much easier to
avoid such bugs if the code documented this.  Like

		u32 *filter_mask = kzalloc(4 * sizeof(u32) * N, GFP_KERNEL);
 		u32 command[N/4];
  		u32 offset[N/4];
  		u32 crc[N/2];


And even better if you let the base types be "native" size so you could
avoid all the complicated indexing math:

		u8 *filter_mask = kzalloc(4 * sizeof(u32) * N, GFP_KERNEL);
 		u8 command[N];
  		u8 offset[N];
  		u16 crc[N];

Yes, you will then have to do type conversions when writing to the chip,
but I believe the overall code will be much easier to follow with this

			command[filter/4] |= 0x05UL << ((filter % 4) * 8);
			offset[filter/4] |= 0x00 << ((filter % 4) * 8);
			crc[filter/2] |= smsc_crc(bcast, 6, filter);

replaced by the IMHO more obvious

			command[filter] = 0x05UL;
			offset[filter] = 0x00;
			crc[filter] =  bitrev16(crc16(0xFFFF, bcast, 6));


BTW, the smsc_crc() function cannot work.  It returns a u16 which it
sometimes will attemt to shift 16 bits...

And you don't test the kzalloc() return value.

And if I am really going to be a nitpick (which comes naturally to me
:-), then I don't think you need to allocate anything at all.  You never
set more than a few bits in the first byte of the filter mask.  Why not
create a small helper function which writes a filter mask with these
bits and fill the rest with zeroes?

Something along the lines of

int write_filter(struct usbnet *dev, u8 firstbyte)
{
        int i, ret = 0;
        u32 v = (u32)firstbyte << 24;

        for (i = 0; i < 4 && !ret; i++) {
                ret = smsc95xx_write_reg_nopm(dev, WUFF, v);
                v = 0;
        }
        return ret;
}

    u8 filter_mask_byte[N];
..
    filter_mask_byte[filter] = 0x3F;
..
    for (i = 0; i < wuff_filter_count; i++) {
	ret = write_filter(dev, filter_mask_byte[i]);
	check_warn_return(ret, "Error writing WUFF\n");
    }
   





Bjørn

Re: [PATCH] smsc95xx: fix suspend buffer overflow

[Joe Perches]

On Tue, 2012-11-27 at 13:23 +0000, Steve Glendinning wrote:
> This patch fixes a buffer overflow introduced by bbd9f9e, where
> the filter_mask array is accessed beyond its bounds.
> 
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Steve Glendinning <steve.glendinning@shawell.net>
> ---
>  drivers/net/usb/smsc95xx.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
> index 79d495d..6cdc504 100644
> --- a/drivers/net/usb/smsc95xx.c
> +++ b/drivers/net/usb/smsc95xx.c
> @@ -1281,7 +1281,7 @@ static int smsc95xx_suspend(struct usb_interface *intf, pm_message_t message)
>  	}
>  
>  	if (pdata->wolopts & (WAKE_BCAST | WAKE_MCAST | WAKE_ARP | WAKE_UCAST)) {
> -		u32 *filter_mask = kzalloc(32, GFP_KERNEL);
> +		u32 *filter_mask = kzalloc(sizeof(u32) * 32, GFP_KERNEL);

It's also unchecked for alloc failure.

>  		u32 command[2];
>  		u32 offset[2];
>  		u32 crc[4];

Re: [PATCH] smsc95xx: fix suspend buffer overflow

[Steve Glendinning]

>>       if (pdata->wolopts & (WAKE_BCAST | WAKE_MCAST | WAKE_ARP | WAKE_UCAST)) {
>> -             u32 *filter_mask = kzalloc(32, GFP_KERNEL);
>> +             u32 *filter_mask = kzalloc(sizeof(u32) * 32, GFP_KERNEL);
>
> It's also unchecked for alloc failure.

Thanks both, I've resubmitted with an alloc failure check,

I completely agree - that filter code isn't pretty!  If you have time
to knock up a patch I'd be happy to test it.

Steve

Re: [PATCH] smsc95xx: fix suspend buffer overflow

[Joe Perches]

On Wed, 2012-11-28 at 18:06 +0000, Steve Glendinning wrote:
> that filter code isn't pretty!  If you have time
> to knock up a patch I'd be happy to test it.

Looking a bit at the code, I don't know how it's supposed to work.

This function seems broken:

static u16 smsc_crc(const u8 *buffer, size_t len, int filter)
{
        return bitrev16(crc16(0xFFFF, buffer, len)) << ((filter % 2) * 16);
}

It always returns 0 when filter is odd.

I imagine 2 things:
o It should return u32
o when multiple WAKE_<foo> flags are set,
  the code doesn't work properly.