Date: Thu, 23 May 2024 23:21:55 +0800
From: shichao lai <shichaorai@...il.com>
To: Alan Stern <stern@...land.harvard.edu>, gregkh@...uxfoundation.org
Cc: oneukum@...e.com, linux-usb@...r.kernel.org, 
	usb-storage@...ts.one-eyed-alien.net, linux-kernel@...r.kernel.org, 
	xingwei lee <xrivendell7@...il.com>, yue sun <samsun1006219@...il.com>
Subject: Re: [PATCHv2] Check whether divisor is non-zero before division

On Thu, May 23, 2024 at 11:13 PM shichao lai <shichaorai@...il.com> wrote:
>
> On Thu, May 23, 2024 at 10:15 PM Alan Stern <stern@...land.harvard.edu> wrote:
> >
> > On Thu, May 23, 2024 at 09:58:21AM -0400, Alan Stern wrote:
> > > On Thu, May 23, 2024 at 05:26:08PM +0800, Shichao Lai wrote:
> > > > Since uzonesize may be zero, so judgements for non-zero are nessesary in both place.
> > > > Previous check is moved out of loop, and one more check is added in alauda_write_lba.
> > > >
> > > > Reported-by: xingwei lee <xrivendell7@...il.com>
> > > > Reported-by: yue sun <samsun1006219@...il.com>
> > > > Signed-off-by: Shichao Lai <shichaorai@...il.com>
> > > > ---
> > > >  drivers/usb/storage/alauda.c | 4 ++++
> > > >  1 file changed, 4 insertions(+)
> > > >
> > > > diff --git a/drivers/usb/storage/alauda.c b/drivers/usb/storage/alauda.c
> > > > index 115f05a6201a..a6e60ef5cb0d 100644
> > > > --- a/drivers/usb/storage/alauda.c
> > > > +++ b/drivers/usb/storage/alauda.c
> > > > @@ -818,6 +818,8 @@ static int alauda_write_lba(struct us_data *us, u16 lba,
> > > >     unsigned int blocksize = MEDIA_INFO(us).blocksize;
> > > >     unsigned int lba_offset = lba % uzonesize;
> > > >     unsigned int new_pba_offset;
> > > > +   if (!uzonesize)
> > > > +           return USB_STOR_TRANSPORT_ERROR;
> > > >     unsigned int zone = lba / uzonesize;
> > > >
> > > >     alauda_ensure_map_for_zone(us, zone);
> > > > @@ -923,6 +925,8 @@ static int alauda_read_data(struct us_data *us, unsigned long address,
> > > >     unsigned int uzonesize = MEDIA_INFO(us).uzonesize;
> > > >     struct scatterlist *sg;
> > > >     int result;
> > > > +   if (!uzonesize)
> > > > +           return USB_STOR_TRANSPORT_ERROR;
> > > >
> > > >     /*
> > > >      * Since we only read in one block at a time, we have to create
> > >
> > > This is definitely NOT the right way to fix the bug!
> > >
> > > uzonesize is set once, when the device is probed, in
> > > alauda_init_media().  That is where the check belongs; if uzonesize is 0
> > > then the function should print a warning and return
> > > USB_STOR_TRANSPORT_ERROR, because the device is unusable.
> > >
> > > It's probably a good idea to check pagesize, blocksize, and zonesize at
> > > the same time, even though none of them are used for any divisions.
> >
> > Wait a minute.  I just went through the code more carefully.  It should
> > not be possible for uzonesize to be 0, because it is defined by:
> >
> >         MEDIA_INFO(us).uzonesize = ((1 << media_info->zoneshift) / 128) * 125;
> >
> > where media_info->zoneshift is always a value between 8 and 12.
> >
> > So the whole idea behind this patch is misguided.  The real problem is
> > to find out why uzonesize ended up being 0.
> >
> > (And it's not necessary to check pagesize, blocksize, or zonesize,
> > because none of them can ever be 0 either.)
> >
> > Alan Stern
>
> Thanks for your comprehensive analysis.
> I added some pr_info() to check the workflow, and I found that the
> uzonesize was not initialized in fact!
>
> The workflow is shown as below.
> Before alauda_read_data(), there are in fact many alauda_check_media(),
> but none of them enter the branch of alauda_init_media(), where
> uzonesize is set to nonzero value.
> The key branch condition is "status[0] & 0x08", which is always
> unsatisfied in this repro.
>
> ```
> alauda_transport
>     alauda_check_media
>         if (status[0] & 0x08) // not satisfied
>             alauda_init_media()
>                 // initialize uzonesize
>     alauda_read_data
> ```
>
> I also print status[0] before the branch, which may be helpful for you
> to analyze.
>
> The part you should focus on is the information beginning with
> "alauda_check_media".
> e.g. "alauda_check_media: before alauda_get_media_status, status[0]:
> 0000000000000000" means in alauda_check_media(), before calling
> alauda_get_media_status()
> It seems that alauda_get_media_status() will transform the status[0]
> to 0x0000000000000036, which doesn't satisfy the condition of
> "status[0] & 0x08".
> ===========
> root@...kaller:~# ./exp
> [   28.645451][ T2386] usb 1-1: new high-speed USB device number 2
> using dummy_hcd
> [   28.885289][ T2386] usb 1-1: Using ep0 maxpacket: 16
> [   29.005519][ T2386] usb 1-1: config 0 has an invalid interface
> number: 192 but max is 0
> [   29.007448][ T2386] usb 1-1: config 0 has no interface number 0
> [   29.008759][ T2386] usb 1-1: config 0 interface 192 altsetting 0
> endpoint 0x9 has invalid wMaxPacketSize 0
> [   29.010799][ T2386] usb 1-1: config 0 interface 192 altsetting 0
> bulk endpoint 0x9 has invalid maxpacket 0
> [   29.012467][ T2386] usb 1-1: config 0 interface 192 altsetting 0
> endpoint 0x8F has invalid maxpacket 59960, setting to 1024
> [   29.012959][ T2386] usb 1-1: config 0 interface 192 altsetting 0
> bulk endpoint 0x8F has invalid maxpacket 1024
> [   29.013413][ T2386] usb 1-1: New USB device found, idVendor=07b4,
> idProduct=010a, bcdDevice= 1.02
> [   29.013809][ T2386] usb 1-1: New USB device strings: Mfr=0,
> Product=0, SerialNumber=0
> [   29.015193][ T2386] usb 1-1: config 0 descriptor??
> [   29.035791][ T4124] raw-gadget.0 gadget.0: fail, usb_ep_enable returned -22
> [   29.057576][ T2386] ums-alauda 1-1:0.192: USB Mass Storage device detected
> [   29.062276][ T2386] scsi host2: usb-storage 1-1:0.192
> [   30.098931][ T2386] scsi 2:0:0:0: Direct-Access     Olympus
> MAUSB-10 (Alauda 0102 PQ: 0 ANSI: 0 CCS
> [   30.102903][ T4131] alauda_check_media: before
> alauda_get_media_status, status[0]: 0000000000000000
> [   30.104297][ T2386] sd 2:0:0:0: Attached scsi generic sg2 type 0
> [   30.135805][ T4131] alauda_get_media_status: data=54, rc=0
> [   30.137113][ T4131] alauda_check_media: after
> alauda_get_media_status, status[0]: 0000000000000036
> [   30.138991][ T4131] alauda_check_media: before init_media,
> status[0]: 0000000000000036
> [   30.141727][ T4131] alauda_check_media: before
> alauda_get_media_status, status[0]: 0000000000000036
> [   30.355544][ T4131] alauda_get_media_status: data=54, rc=0
> [   30.356815][ T4131] alauda_check_media: after
> alauda_get_media_status, status[0]: 0000000000000036
> [   30.358728][ T4131] alauda_check_media: before init_media,
> status[0]: 0000000000000036
> [   30.361408][ T4131] alauda_check_media: before
> alauda_get_media_status, status[0]: 00000000000000ff
> [   30.575607][ T4131] alauda_get_media_status: data=54, rc=0
> [   30.576910][ T4131] alauda_check_media: after
> alauda_get_media_status, status[0]: 0000000000000036
> [   30.578823][ T4131] alauda_check_media: before init_media,
> status[0]: 0000000000000036
> [   30.580999][ T4131] alauda_check_media: before
> alauda_get_media_status, status[0]: 00000000000000ff
> [   30.795625][ T4131] alauda_get_media_status: data=54, rc=0
> [   30.796899][ T4131] alauda_check_media: after
> alauda_get_media_status, status[0]: 0000000000000036
> [   30.798773][ T4131] alauda_check_media: before init_media,
> status[0]: 0000000000000036
> [   30.801017][ T4131] alauda_check_media: before
> alauda_get_media_status, status[0]: 00000000000000ff
> [   31.015362][ T4131] alauda_get_media_status: data=54, rc=0
> [   31.016679][ T4131] alauda_check_media: after
> alauda_get_media_status, status[0]: 0000000000000036
> [   31.018565][ T4131] alauda_check_media: before init_media,
> status[0]: 0000000000000036
> [   31.020543][  T263] sd 2:0:0:0: [sdb] Very big device. Trying to
> use READ CAPACITY(16).
> [   31.023323][  T263] sd 2:0:0:0: [sdb] Using 0xffffffff as device size
> [   31.035382][ T2386] scsi 2:0:0:1: Direct-Access     Olympus
> MAUSB-10 (Alauda 0102 PQ: 0 ANSI: 0 CCS
> [   31.035466][ T4131] alauda_transport: before alauda_check_media
> [   31.038810][ T4131] alauda_check_media: before
> alauda_get_media_status, status[0]: 0000000000000000
> [   31.043663][ T2386] sd 2:0:0:1: Attached scsi generic sg3 type 0
> [   31.235486][ T4131] alauda_get_media_status: data=54, rc=0
> [   31.237015][ T4131] alauda_check_media: after
> alauda_get_media_status, status[0]: 0000000000000036
> [   31.239266][ T4131] alauda_check_media: before init_media,
> status[0]: 0000000000000036
> [   31.241273][ T4131] alauda_transport: after alauda_check_media ->
> alauda_read_data
> [   31.243134][ T4131] alauda_read_data: 0
> [   31.244148][ T4131] divide error: 0000 [#1] PREEMPT SMP KASAN NOPTI

For ease of reproduction, I attach my kernel config and repro.c.
The kernel version is v6.9-rc7
gcc version: 11.4.0

View attachment "repro.c" of type "text/x-csrc" (21518 bytes)

Download attachment "linux-v6.9_usb_config" of type "application/octet-stream" (233412 bytes)

Powered by blists - more mailing lists