Received-SPF: pass (google.com: domain of linux-kernel+bounces-88366-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Precedence: bulk
MIME-Version: 1.0
References: <380909e4-6e0a-402f-b3ac-de07e520c910@rowland.harvard.edu>
 <000000000000102fe606127a67f6@google.com> <b1e605ea-333f-4ac0-9511-da04f411763e@rowland.harvard.edu>
In-Reply-To: <b1e605ea-333f-4ac0-9511-da04f411763e@rowland.harvard.edu>
From: PrasannaKumar Muralidharan <prasannatsmkumar@gmail.com>
Date: Fri, 1 Mar 2024 17:06:56 +0530
Message-ID: <CANc+2y6FAoUR7R_N-3aLB72DBN98gx82vLMAKXRSCtCQ8PoJEQ@mail.gmail.com>
Subject: Re: [PATCH] USB: usb-storage: Prevent divide-by-0 error in isd200_ata_command
To: Alan Stern <stern@rowland.harvard.edu>
Cc: Greg KH <gregkh@linuxfoundation.org>, 
	syzbot <syzbot+28748250ab47a8f04100@syzkaller.appspotmail.com>, bvanassche@acm.org, 
	emilne@redhat.com, linux-kernel@vger.kernel.org, linux-usb@vger.kernel.org, 
	martin.petersen@oracle.com, nogikh@google.com, 
	syzkaller-bugs@googlegroups.com, tasos@tasossah.com, 
	usb-storage@lists.one-eyed-alien.net
Content-Type: text/plain; charset="UTF-8"

On Fri, 1 Mar 2024 at 01:00, Alan Stern <stern@rowland.harvard.edu> wrote:
>
> The isd200 sub-driver in usb-storage uses the HEADS and SECTORS values
> in the ATA ID information to calculate cylinder and head values when
> creating a CDB for READ or WRITE commands.  The calculation involves
> division and modulus operations, which will cause a crash if either of
> these values is 0.  While this never happens with a genuine device, it
> could happen with a flawed or subversive emulation, as reported by the
> syzbot fuzzer.
>
> Protect against this possibility by refusing to bind to the device if
> either the ATA_ID_HEADS or ATA_ID_SECTORS value in the device's ID
> information is 0.  This requires isd200_Initialization() to return a
> negative error code when initialization fails; currently it always
> returns 0 (even when there is an error).
>
> Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
> Reported-and-tested-by: syzbot+28748250ab47a8f04100@syzkaller.appspotmail.com
> Link: https://lore.kernel.org/linux-usb/0000000000003eb868061245ba7f@google.com/
> Fixes: 1da177e4c3f4 ("v2.6.12-rc2")
> Cc: <stable@vger.kernel.org>
>
> ---
>
>  drivers/usb/storage/isd200.c |   23 ++++++++++++++++++-----
>  1 file changed, 18 insertions(+), 5 deletions(-)
>
> Index: usb-devel/drivers/usb/storage/isd200.c
> ===================================================================
> --- usb-devel.orig/drivers/usb/storage/isd200.c
> +++ usb-devel/drivers/usb/storage/isd200.c
> @@ -1105,7 +1105,7 @@ static void isd200_dump_driveid(struct u
>  static int isd200_get_inquiry_data( struct us_data *us )
>  {
>         struct isd200_info *info = (struct isd200_info *)us->extra;
> -       int retStatus = ISD200_GOOD;
> +       int retStatus;
>         u16 *id = info->id;
>
>         usb_stor_dbg(us, "Entering isd200_get_inquiry_data\n");
> @@ -1137,6 +1137,13 @@ static int isd200_get_inquiry_data( stru
>                                 isd200_fix_driveid(id);
>                                 isd200_dump_driveid(us, id);
>
> +                               /* Prevent division by 0 in isd200_scsi_to_ata() */
> +                               if (id[ATA_ID_HEADS] == 0 || id[ATA_ID_SECTORS] == 0) {
> +                                       usb_stor_dbg(us, "   Invalid ATA Identify data\n");
> +                                       retStatus = ISD200_ERROR;
> +                                       goto Done;
> +                               }
> +
>                                 memset(&info->InquiryData, 0, sizeof(info->InquiryData));
>
>                                 /* Standard IDE interface only supports disks */
> @@ -1202,6 +1209,7 @@ static int isd200_get_inquiry_data( stru
>                 }
>         }
>
> + Done:
>         usb_stor_dbg(us, "Leaving isd200_get_inquiry_data %08X\n", retStatus);
>
>         return(retStatus);
> @@ -1481,22 +1489,27 @@ static int isd200_init_info(struct us_da
>
>  static int isd200_Initialization(struct us_data *us)
>  {
> +       int rc = 0;
> +
>         usb_stor_dbg(us, "ISD200 Initialization...\n");
>
>         /* Initialize ISD200 info struct */
>
> -       if (isd200_init_info(us) == ISD200_ERROR) {
> +       if (isd200_init_info(us) < 0) {
>                 usb_stor_dbg(us, "ERROR Initializing ISD200 Info struct\n");
> +               rc = -ENOMEM;
>         } else {
>                 /* Get device specific data */
>
> -               if (isd200_get_inquiry_data(us) != ISD200_GOOD)
> +               if (isd200_get_inquiry_data(us) != ISD200_GOOD) {
>                         usb_stor_dbg(us, "ISD200 Initialization Failure\n");
> -               else
> +                       rc = -EINVAL;
> +               } else {
>                         usb_stor_dbg(us, "ISD200 Initialization complete\n");
> +               }
>         }
>
> -       return 0;
> +       return rc;
>  }
>
>
>

Looks good to me.

Reviewed-by: PrasannaKumar Muralidharan <prasannatsmkumar@gmail.com>

Regards,
PrasannaKumar