| Message ID | 20240526012745.2852061-1-shichaorai@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 16637fea001ab3c8df528a8995b3211906165a30 |
| Headers | show |
| Series | [v6] usb-storage: alauda: Check whether the media is initialized | expand |
… > Fixes: e80b0fade09e ("[PATCH] USB Storage: add alauda support") How do you think about to omit the text “[PATCH] ” from the tag summary? > Reported-by: xingwei lee <xrivendell7@gmail.com> > Reported-by: yue sun <samsun1006219@gmail.com> Would you like to add any links as background information for these reports? Regards, Markus
On Sun, May 26, 2024 at 08:49:02AM +0200, Markus Elfring wrote: > … > > Fixes: e80b0fade09e ("[PATCH] USB Storage: add alauda support") > > How do you think about to omit the text “[PATCH] ” from the tag summary? Then it would be incorrect. Again, Markus, please STOP sending review comments that are obviously wrong. New developers do not know who to ignore and you are telling them things that are wrong and not helpful at all. Please stop reviewing patches, this is not ok and is actually harmful to our community. greg k-h
>> … >>> Fixes: e80b0fade09e ("[PATCH] USB Storage: add alauda support") >> >> How do you think about to omit the text “[PATCH] ” from the tag summary? > > Then it would be incorrect. I find this view interesting. > Again, Markus, please STOP sending review comments that are obviously wrong. The mentioned tag needs an “one line summary”. https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.9#n145 Do you find it required to repeat a questionable commit subject completely? > New developers do not know who to ignore and you are telling > them things that are wrong and not helpful at all. Additional development views can occasionally become helpful. > Please stop reviewing patches, Patch review is one of the usual software development activities. > this is not ok I suggest to reconsider such a view once more. > is actually harmful to our community. Possible improvements are varying between affected software components. Regards, Markus
On Sun, May 26, 2024 at 11:20:23AM +0200, Markus Elfring wrote: > >> … > >>> Fixes: e80b0fade09e ("[PATCH] USB Storage: add alauda support") > >> > >> How do you think about to omit the text “[PATCH] ” from the tag summary? > > > > Then it would be incorrect. > > I find this view interesting. It is not "interesting" when you tell people things that are flat out wrong and trivial to prove wrong. You are doing nothing to help here, please stop or we are going to have to ban you from our community, again. greg k-h
On 26.05.24 03:27, Shichao Lai wrote: Hi, > The member "uzonesize" of struct alauda_info will remain 0 > if alauda_init_media() fails, potentially causing divide errors > in alauda_read_data() and alauda_write_lba(). This means that we can reach those functions. > - Add a member "media_initialized" to struct alauda_info. > - Change a condition in alauda_check_media() to ensure the > first initialization. > - Add an error check for the return value of alauda_init_media(). > > Fixes: e80b0fade09e ("[PATCH] USB Storage: add alauda support") > Reported-by: xingwei lee <xrivendell7@gmail.com> > Reported-by: yue sun <samsun1006219@gmail.com> > Reviewed-by: Alan Stern <stern@rowland.harvard.edu> > Signed-off-by: Shichao Lai <shichaorai@gmail.com> > --- > Changes since v5: > - Check the initialization of alauda_check_media() > which is the root cause. > > drivers/usb/storage/alauda.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/drivers/usb/storage/alauda.c b/drivers/usb/storage/alauda.c > index 115f05a6201a..40d34cc28344 100644 > --- a/drivers/usb/storage/alauda.c > +++ b/drivers/usb/storage/alauda.c > @@ -105,6 +105,8 @@ struct alauda_info { > unsigned char sense_key; > unsigned long sense_asc; /* additional sense code */ > unsigned long sense_ascq; /* additional sense code qualifier */ > + > + bool media_initialized; > }; > > #define short_pack(lsb,msb) ( ((u16)(lsb)) | ( ((u16)(msb))<<8 ) ) > @@ -476,11 +478,12 @@ static int alauda_check_media(struct us_data *us) > } > > /* Check for media change */ > - if (status[0] & 0x08) { > + if (status[0] & 0x08 || !info->media_initialized) { If we take this branch due to !info->media_initialized and only due to this condition, alauda_check_media() will return an error (Quoting the current state): /* Check for media change */ if (status[0] & 0x08) { usb_stor_dbg(us, "Media change detected\n"); alauda_free_maps(&MEDIA_INFO(us)); alauda_init_media(us); info->sense_key = UNIT_ATTENTION; info->sense_asc = 0x28; info->sense_ascq = 0x00; return USB_STOR_TRANSPORT_FAILED; > usb_stor_dbg(us, "Media change detected\n"); > alauda_free_maps(&MEDIA_INFO(us)); > - alauda_init_media(us); > - > + rc = alauda_init_media(us); > + if (rc == USB_STOR_TRANSPORT_GOOD) > + info->media_initialized = true; > info->sense_key = UNIT_ATTENTION; > info->sense_asc = 0x28; > info->sense_ascq = 0x00; It seems to that we need to evaluate the reasons for taking this branch and the result of alauda_init_media() to compute the correct return of this function. Regards Oliver
On Mon, May 27, 2024 at 05:01:13PM +0200, Oliver Neukum wrote: > On 26.05.24 03:27, Shichao Lai wrote: > > Hi, > > > > The member "uzonesize" of struct alauda_info will remain 0 > > if alauda_init_media() fails, potentially causing divide errors > > in alauda_read_data() and alauda_write_lba(). > > This means that we can reach those functions. > > > - Add a member "media_initialized" to struct alauda_info. > > - Change a condition in alauda_check_media() to ensure the > > first initialization. > > - Add an error check for the return value of alauda_init_media(). > > > > Fixes: e80b0fade09e ("[PATCH] USB Storage: add alauda support") > > Reported-by: xingwei lee <xrivendell7@gmail.com> > > Reported-by: yue sun <samsun1006219@gmail.com> > > Reviewed-by: Alan Stern <stern@rowland.harvard.edu> > > Signed-off-by: Shichao Lai <shichaorai@gmail.com> > > --- > > Changes since v5: > > - Check the initialization of alauda_check_media() > > which is the root cause. > > > > drivers/usb/storage/alauda.c | 9 ++++++--- > > 1 file changed, 6 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/usb/storage/alauda.c b/drivers/usb/storage/alauda.c > > index 115f05a6201a..40d34cc28344 100644 > > --- a/drivers/usb/storage/alauda.c > > +++ b/drivers/usb/storage/alauda.c > > @@ -105,6 +105,8 @@ struct alauda_info { > > unsigned char sense_key; > > unsigned long sense_asc; /* additional sense code */ > > unsigned long sense_ascq; /* additional sense code qualifier */ > > + > > + bool media_initialized; > > }; > > #define short_pack(lsb,msb) ( ((u16)(lsb)) | ( ((u16)(msb))<<8 ) ) > > @@ -476,11 +478,12 @@ static int alauda_check_media(struct us_data *us) > > } > > /* Check for media change */ > > - if (status[0] & 0x08) { > > + if (status[0] & 0x08 || !info->media_initialized) { > > If we take this branch due to !info->media_initialized and only due > to this condition, alauda_check_media() will return an error > > (Quoting the current state): > /* Check for media change */ > if (status[0] & 0x08) { > usb_stor_dbg(us, "Media change detected\n"); > alauda_free_maps(&MEDIA_INFO(us)); > alauda_init_media(us); > > info->sense_key = UNIT_ATTENTION; > info->sense_asc = 0x28; > info->sense_ascq = 0x00; > return USB_STOR_TRANSPORT_FAILED; Indeed. That's what would happen with a properly functioning device, as opposed to a malicious one or a purposely defective fuzzing emulation. The point of the patch is to make the system treat these other things in the same way as it treats normal devices. > > usb_stor_dbg(us, "Media change detected\n"); > > alauda_free_maps(&MEDIA_INFO(us)); > > - alauda_init_media(us); > > - > > + rc = alauda_init_media(us); > > + if (rc == USB_STOR_TRANSPORT_GOOD) > > + info->media_initialized = true; > > info->sense_key = UNIT_ATTENTION; > > info->sense_asc = 0x28; > > info->sense_ascq = 0x00; > > It seems to that we need to evaluate the reasons for taking this branch > and the result of alauda_init_media() to compute the correct return > of this function. The return value is what it should be. With a normal device: We see that the device reports a media change. We read the characteristics of the new media and report a UNIT ATTENTION error, notifyng the SCSI layer about the new media and forcing it to retry the command. With the defective syzbot emulation and the original code: We don't see a report of a media change, so we try to carry out a READ or WRITE operation without knowing any of the media characteristics. Result: division by zero. With the defective syzbot emulation and the patched code: We don't see a report of a media change, but we do see that the media hasn't been initialized, so we try to read the characteristics of the media. This fails, so the media_initialized flag remains clear and the command continues to fail until the SCSI layer gives up. (Although maybe it would be better to report a different error to the SCSI layer when this happens; UNIT ATTENTION with 0x28: Media May Have Changed doesn't seem right.) Alan Stern
diff --git a/drivers/usb/storage/alauda.c b/drivers/usb/storage/alauda.c index 115f05a6201a..40d34cc28344 100644 --- a/drivers/usb/storage/alauda.c +++ b/drivers/usb/storage/alauda.c @@ -105,6 +105,8 @@ struct alauda_info { unsigned char sense_key; unsigned long sense_asc; /* additional sense code */ unsigned long sense_ascq; /* additional sense code qualifier */ + + bool media_initialized; }; #define short_pack(lsb,msb) ( ((u16)(lsb)) | ( ((u16)(msb))<<8 ) ) @@ -476,11 +478,12 @@ static int alauda_check_media(struct us_data *us) } /* Check for media change */ - if (status[0] & 0x08) { + if (status[0] & 0x08 || !info->media_initialized) { usb_stor_dbg(us, "Media change detected\n"); alauda_free_maps(&MEDIA_INFO(us)); - alauda_init_media(us); - + rc = alauda_init_media(us); + if (rc == USB_STOR_TRANSPORT_GOOD) + info->media_initialized = true; info->sense_key = UNIT_ATTENTION; info->sense_asc = 0x28; info->sense_ascq = 0x00;
The member "uzonesize" of struct alauda_info will remain 0 if alauda_init_media() fails, potentially causing divide errors in alauda_read_data() and alauda_write_lba(). - Add a member "media_initialized" to struct alauda_info. - Change a condition in alauda_check_media() to ensure the first initialization. - Add an error check for the return value of alauda_init_media(). Fixes: e80b0fade09e ("[PATCH] USB Storage: add alauda support") Reported-by: xingwei lee <xrivendell7@gmail.com> Reported-by: yue sun <samsun1006219@gmail.com> Reviewed-by: Alan Stern <stern@rowland.harvard.edu> Signed-off-by: Shichao Lai <shichaorai@gmail.com> --- Changes since v5: - Check the initialization of alauda_check_media() which is the root cause. drivers/usb/storage/alauda.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)