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 <[email protected]>
Reported-by: yue sun <[email protected]>
Reviewed-by: Alan Stern <[email protected]>
Signed-off-by: Shichao Lai <[email protected]>
---
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) {
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;
--
2.34.1
…
> 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 <[email protected]>
> Reported-by: yue sun <[email protected]>
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 <[email protected]>
> Reported-by: yue sun <[email protected]>
> Reviewed-by: Alan Stern <[email protected]>
> Signed-off-by: Shichao Lai <[email protected]>
> ---
> 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 <[email protected]>
> > Reported-by: yue sun <[email protected]>
> > Reviewed-by: Alan Stern <[email protected]>
> > Signed-off-by: Shichao Lai <[email protected]>
> > ---
> > 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