| Message ID | 20240523092608.874986-1-shichaorai@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [PATCHv2] Check whether divisor is non-zero before division | expand |
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@gmail.com> > Reported-by: yue sun <samsun1006219@gmail.com> > Signed-off-by: Shichao Lai <shichaorai@gmail.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 > -- > 2.34.1 > > Hi, This is the friendly patch-bot of Greg Kroah-Hartman. You have sent him a patch that has triggered this response. He used to manually respond to these common problems, but in order to save his sanity (he kept writing the same thing over and over, yet to different people), I was created. Hopefully you will not take offence and will fix the problem in your patch and resubmit it so that it can be accepted into the Linux kernel tree. You are receiving this message because of the following common error(s) as indicated below: - Your patch contains warnings and/or errors noticed by the scripts/checkpatch.pl tool. - This looks like a new version of a previously submitted patch, but you did not list below the --- line any changes from the previous version. Please read the section entitled "The canonical patch format" in the kernel file, Documentation/process/submitting-patches.rst for what needs to be done here to properly describe this. If you wish to discuss this problem further, or you have questions about how to resolve this issue, please feel free to respond to this email and Greg will reply once he has dug out from the pending patches received from other developers. thanks, greg k-h's patch email bot
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@gmail.com> > Reported-by: yue sun <samsun1006219@gmail.com> > Signed-off-by: Shichao Lai <shichaorai@gmail.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. Alan Stern
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@gmail.com> > > Reported-by: yue sun <samsun1006219@gmail.com> > > Signed-off-by: Shichao Lai <shichaorai@gmail.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
On Thu, May 23, 2024 at 10:15 PM Alan Stern <stern@rowland.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@gmail.com> > > > Reported-by: yue sun <samsun1006219@gmail.com> > > > Signed-off-by: Shichao Lai <shichaorai@gmail.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@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
On Thu, May 23, 2024 at 11:13 PM shichao lai <shichaorai@gmail.com> wrote: > > On Thu, May 23, 2024 at 10:15 PM Alan Stern <stern@rowland.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@gmail.com> > > > > Reported-by: yue sun <samsun1006219@gmail.com> > > > > Signed-off-by: Shichao Lai <shichaorai@gmail.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@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 For ease of reproduction, I attach my kernel config and repro.c. The kernel version is v6.9-rc7 gcc version: 11.4.0
On Thu, May 23, 2024 at 11:13:08PM +0800, shichao lai wrote: > 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 > ``` Good work! So the problem is that the driver believes the status[0] & 0x08 test. The way to fix this is to add an "initialized" flag to the alauda_info structure. Then alauda_check_media() should call alauda_init_media() if the 0x08 bit is set in status[0] _or_ if info->initialized is 0. And of course, alauda_check_media() should then set info->initialized to 1 if the alauda_init_media() call succeeds. Would you like to write and test a patch that does this? Alan Stern
On Fri, May 24, 2024 at 12:30 AM Alan Stern <stern@rowland.harvard.edu> wrote: > > Good work! So the problem is that the driver believes the status[0] & > 0x08 test. > > The way to fix this is to add an "initialized" flag to the alauda_info > structure. Then alauda_check_media() should call alauda_init_media() if > the 0x08 bit is set in status[0] _or_ if info->initialized is 0. And of > course, alauda_check_media() should then set info->initialized to 1 if > the alauda_init_media() call succeeds. > > Would you like to write and test a patch that does this? > > Alan Stern I tried to do this. And the workflow can enter alauda_init_media(), but there are still many conditions to satisfy in alauda_init_media(). Unfortunately alauda_init_media() stop and return here before initializing uzonesize: if (data[0] != 0x14) { usb_stor_dbg(us, "Media not ready after ack\n"); return USB_STOR_TRANSPORT_ERROR; } The data[0] is status[0] showed before, and it was 0x0036. I am not familiar with the status code of alauda. How can I deal with this condition? Is it ok to pass this condition when info->initialized == false, even if the data[0] != 0x14?
On Fri, May 24, 2024 at 10:13:45AM +0800, shichao lai wrote: > On Fri, May 24, 2024 at 12:30 AM Alan Stern <stern@rowland.harvard.edu> wrote: > > > > Good work! So the problem is that the driver believes the status[0] & > > 0x08 test. > > > > The way to fix this is to add an "initialized" flag to the alauda_info > > structure. Then alauda_check_media() should call alauda_init_media() if > > the 0x08 bit is set in status[0] _or_ if info->initialized is 0. And of > > course, alauda_check_media() should then set info->initialized to 1 if > > the alauda_init_media() call succeeds. > > > > Would you like to write and test a patch that does this? > > > > Alan Stern > > I tried to do this. And the workflow can enter alauda_init_media(), > but there are still many conditions to satisfy in alauda_init_media(). > Unfortunately alauda_init_media() stop and return here before > initializing uzonesize: > > if (data[0] != 0x14) { > usb_stor_dbg(us, "Media not ready after ack\n"); > return USB_STOR_TRANSPORT_ERROR; > } That's an error return. > The data[0] is status[0] showed before, and it was 0x0036. > I am not familiar with the status code of alauda. > How can I deal with this condition? > Is it ok to pass this condition when info->initialized == false, even > if the data[0] != 0x14? If alauda_init_media() returns an error, leave info->initialized unchanged. alauda_check_media() will return an error also, so the bad division won't take place. Alan Stern
On Sat, May 25, 2024 at 10:24 AM Alan Stern <stern@rowland.harvard.edu> wrote: > If alauda_init_media() returns an error, leave info->initialized > unchanged. alauda_check_media() will return an error also, so the bad > division won't take place. > > Alan Stern Thanks! You also remind me that the return value from alauda_init_media() is never used! By this way, the workflow now seems to work correctly. It tries to initialize multiple times, and finally disconnects due to no response. Now if possible, I will post a [PATCH v4] for this bug soon. I want to know whether it is possible to add some tags like Suggested-by or Reviewed-by for Dear Alan Stern, gregkh and oneukum as thanks for your discussions. e.g. ==== kernel log [ 47.266129][ T4125] alauda_check_media: before init_media, status[0]: 0000000000000036 [ 47.266555][ T4125] alauda_check_media: enter init_media [ 47.467314][ T9] usb 1-1: USB disconnect, device number 2 root@syzkaller:~# [ 47.485304][ T4125] alauda_get_media_status: data=54, rc=4 [ 47.485640][ T4125] alauda_init_media: exit in 391 [ 47.486104][ T41] sd 2:0:0:0: [sdb] Read Capacity(10) failed: Result: hostbyte=DID_ERROR driverbyte=DRIVER_OK [ 47.486591][ T41] sd 2:0:0:0: [sdb] Sense not available. [ 47.486889][ T41] sd 2:0:0:0: [sdb] 0 512-byte logical blocks: (0 B/0 B) [ 47.487212][ T41] sd 2:0:0:0: [sdb] 0-byte physical blocks [ 47.487515][ T41] sd 2:0:0:0: [sdb] Write Protect is off [ 47.487813][ T41] sd 2:0:0:0: [sdb] Asking for cache data failed [ 47.488104][ T41] sd 2:0:0:0: [sdb] Assuming drive cache: write through [ 47.491396][ T41] sd 2:0:0:0: [sdb] Attached SCSI removable disk [ 48.105309][ T1198] not responding...
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
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@gmail.com> Reported-by: yue sun <samsun1006219@gmail.com> Signed-off-by: Shichao Lai <shichaorai@gmail.com> --- drivers/usb/storage/alauda.c | 4 ++++ 1 file changed, 4 insertions(+)