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@xxxxxxxxx>
> > Reported-by: yue sun <samsun1006219@xxxxxxxxx>
> > Reviewed-by: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>
> > Signed-off-by: Shichao Lai <shichaorai@xxxxxxxxx>
> > ---
> > 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
