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 <[email protected]>
Reported-by: yue sun <[email protected]>
Signed-off-by: Shichao Lai <[email protected]>
---
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
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 <[email protected]>
> Reported-by: yue sun <[email protected]>
> Signed-off-by: Shichao Lai <[email protected]>
> ---
> 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 <[email protected]>
> Reported-by: yue sun <[email protected]>
> Signed-off-by: Shichao Lai <[email protected]>
> ---
> 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 <[email protected]>
> > Reported-by: yue sun <[email protected]>
> > Signed-off-by: Shichao Lai <[email protected]>
> > ---
> > 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 <[email protected]> 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 <[email protected]>
> > > Reported-by: yue sun <[email protected]>
> > > Signed-off-by: Shichao Lai <[email protected]>
> > > ---
> > > 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 <[email protected]> wrote:
>
> On Thu, May 23, 2024 at 10:15 PM Alan Stern <[email protected]> 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 <[email protected]>
> > > > Reported-by: yue sun <[email protected]>
> > > > Signed-off-by: Shichao Lai <[email protected]>
> > > > ---
> > > > 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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...