[PATCH v6] usb-storage: alauda: Check whether the media is initialized

[PATCH v6] usb-storage: alauda: Check whether the media is initialized

[Shichao Lai]

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(-)

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

Re: [PATCH v6] usb-storage: alauda: Check whether the media is initialized

[Markus Elfring]

…
> 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

Re: [PATCH v6] usb-storage: alauda: Check whether the media is initialized

[Greg Kroah-Hartman]

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

Re: [v6] usb-storage: alauda: Check whether the media is initialized

[Markus Elfring]

>> …
>>> 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

Re: [v6] usb-storage: alauda: Check whether the media is initialized

[Greg Kroah-Hartman]

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

Re: [PATCH v6] usb-storage: alauda: Check whether the media is initialized

[Oliver Neukum]

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

Re: [PATCH v6] usb-storage: alauda: Check whether the media is initialized

[Alan Stern]

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