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  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]
Date:   Tue, 30 Apr 2019 10:13:38 -0400 (EDT)
From:   Alan Stern <stern@...land.harvard.edu>
To:     Eric Biggers <ebiggers@...nel.org>
cc:     syzbot <syzbot+a9fefd18c7b240f19c54@...kaller.appspotmail.com>,
        <andreyknvl@...gle.com>, <gregkh@...uxfoundation.org>,
        <linux-kernel@...r.kernel.org>, <linux-usb@...r.kernel.org>,
        <rafael@...nel.org>, <syzkaller-bugs@...glegroups.com>
Subject: Re: KASAN: slab-out-of-bounds Read in hex_string

On Mon, 29 Apr 2019, Eric Biggers wrote:

> On Mon, Apr 29, 2019 at 04:07:04PM -0400, Alan Stern wrote:

> > Accessing beyond the end of the descriptor.
> > 
> > #syz test: https://github.com/google/kasan.git usb-fuzzer
> > 
> > --- a/drivers/video/fbdev/udlfb.c
> > +++ b/drivers/video/fbdev/udlfb.c
> > @@ -1511,6 +1511,7 @@ static int dlfb_parse_vendor_descriptor(
> >  	char *buf;
> >  	char *desc_end;
> >  	int total_len;
> > +	int width;
> >  
> >  	buf = kzalloc(MAX_VENDOR_DESCRIPTOR_SIZE, GFP_KERNEL);
> >  	if (!buf)
> > @@ -1529,9 +1530,10 @@ static int dlfb_parse_vendor_descriptor(
> >  	}
> >  
> >  	if (total_len > 5) {
> > +		width = min(total_len, 11);
> >  		dev_info(&intf->dev,
> > -			 "vendor descriptor length: %d data: %11ph\n",
> > -			 total_len, desc);
> > +			 "vendor descriptor length: %d data: %*ph\n",
> > +			 total_len, width, desc);
> >  
> >  		if ((desc[0] != total_len) || /* descriptor length */
> >  		    (desc[1] != 0x5f) ||   /* vendor descriptor type */
> > 
> > 
> 
> Why not write just:
> 
>                 dev_info(&intf->dev,
>                          "vendor descriptor length: %d data: %*ph\n",
>                          total_len, min(total_len, 11), desc);

I did consider doing that.  In the end I decided adding an extra
temporary variable made the code a little more readable.  (For some 
reason, extra recursion -- a function call embedded in a function 
argument -- seems to demand more mental effort than an extra 
temporary.  Maybe my brain is just getting old...)

> Also, aren't there more out-of-bounds reads in the code just after?  It only
> checks for at least 1 byte available, but then it reads up to 7 bytes:
> 
> 		while (desc < desc_end) {
> 			u8 length;
> 			u16 key;
> 
> 			key = *desc++;
> 			key |= (u16)*desc++ << 8;
> 			length = *desc++;
> 
> 			switch (key) {
> 			case 0x0200: { /* max_area */
> 				u32 max_area = *desc++;
> 				max_area |= (u32)*desc++ << 8;
> 				max_area |= (u32)*desc++ << 16;
> 				max_area |= (u32)*desc++ << 24;
> 				dev_warn(&intf->dev,
> 					 "DL chip limited to %d pixel modes\n",
> 					 max_area);
> 				dlfb->sku_pixel_limit = max_area;
> 				break;
> 			}
> 			default:
> 				break;
> 			}
> 			desc += length;
> 		}

Quite right.  Please feel free to submit a patch fixing all these 
problems.

> Also I couldn't help but notice it's also using 'char' rather than 'u8',
> so bytes >= 0x80 are read incorrectly as they're sign extended...

As I recall, the C standard doesn't specify whether char is signed or
unsigned; it can vary with the implementation.  However you are
certainly correct that to ensure there is no sign extension, the code
should use unsigned char or u8.

Alan Stern

Powered by blists - more mailing lists