netdev - Re: [PATCH] smsc95xx: fix suspend buffer overflow

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87a9u39ks1.fsf@nemi.mork.no>
Date:	Tue, 27 Nov 2012 15:34:54 +0100
From:	Bjørn Mork <bjorn@...k.no>
To:	Steve Glendinning <steve.glendinning@...well.net>
Cc:	netdev@...r.kernel.org, dan.carpenter@...cle.com
Subject: Re: [PATCH] smsc95xx: fix suspend buffer overflow

Steve Glendinning <steve.glendinning@...well.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@...cle.com>
> Signed-off-by: Steve Glendinning <steve.glendinning@...well.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
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html