Re: [PATCHv2] Check whether divisor is non-zero before division

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, May 23, 2024 at 10:15 PM Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxx>
> > > Reported-by: yue sun <samsun1006219@xxxxxxxxx>
> > > Signed-off-by: Shichao Lai <shichaorai@xxxxxxxxx>
> > > ---
> > >  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@syzkaller:~# ./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





[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux