USB: usb-storage: Prevent divide-by-0 error in isd200_ata_command
Commit Message
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(-)
Comments
Alan,
> 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.
Looks good to me.
Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com>
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
===================================================================
@@ -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;
}