[syzbot] [usb-storage?] divide error in isd200_ata_command

[syzbot] [usb-storage?] divide error in isd200_ata_command

[syzbot]

Hello,

syzbot found the following issue on:

HEAD commit:    f2e367d6ad3b Merge tag 'for-6.8/dm-fix-3' of git://git.ker..
git tree:       upstream
console+strace: https://syzkaller.appspot.com/x/log.txt?x=114e10e4180000
kernel config:  https://syzkaller.appspot.com/x/.config?x=eff9f3183d0a20dd
dashboard link: https://syzkaller.appspot.com/bug?extid=28748250ab47a8f04100
compiler:       Debian clang version 15.0.6, GNU ld (GNU Binutils for Debian) 2.40
syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=1064b372180000
C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=10aca6ac180000

Downloadable assets:
disk image: https://storage.googleapis.com/syzbot-assets/c55ca1fdc5ad/disk-f2e367d6.raw.xz
vmlinux: https://storage.googleapis.com/syzbot-assets/4556a82fb4ed/vmlinux-f2e367d6.xz
kernel image: https://storage.googleapis.com/syzbot-assets/95338ed9dad1/bzImage-f2e367d6.xz

The issue was bisected to:

commit 321da3dc1f3c92a12e3c5da934090d2992a8814c
Author: Martin K. Petersen <martin.petersen@oracle.com>
Date:   Tue Feb 13 14:33:06 2024 +0000

    scsi: sd: usb_storage: uas: Access media prior to querying device properties

bisection log:  https://syzkaller.appspot.com/x/bisect.txt?x=15a3934a180000
final oops:     https://syzkaller.appspot.com/x/report.txt?x=17a3934a180000
console output: https://syzkaller.appspot.com/x/log.txt?x=13a3934a180000

IMPORTANT: if you fix the issue, please add the following tag to the commit:
Reported-by: syzbot+28748250ab47a8f04100@syzkaller.appspotmail.com
Fixes: 321da3dc1f3c ("scsi: sd: usb_storage: uas: Access media prior to querying device properties")

divide error: 0000 [#1] PREEMPT SMP KASAN PTI
CPU: 0 PID: 5070 Comm: usb-storage Not tainted 6.8.0-rc5-syzkaller-00297-gf2e367d6ad3b #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/25/2024
RIP: 0010:isd200_scsi_to_ata drivers/usb/storage/isd200.c:1318 [inline]
RIP: 0010:isd200_ata_command+0x776/0x2380 drivers/usb/storage/isd200.c:1529
Code: 62 fa 49 8d 7c 24 0c 48 89 f8 48 c1 e8 03 42 0f b6 04 28 84 c0 0f 85 00 18 00 00 41 0f b7 5c 24 0c 48 8b 7c 24 18 89 f8 31 d2 <f7> f3 41 89 d0 49 83 c4 06 4c 89 e0 48 c1 e8 03 42 0f b6 04 28 84
RSP: 0018:ffffc900043ffc00 EFLAGS: 00010246
RAX: 0000000000000000 RBX: 0000000000000000 RCX: ffff888023230000
RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000000
RBP: ffffc900043ffd50 R08: ffffffff873161fd R09: ffffffff87315c95
R10: 0000000000000008 R11: ffff888023230000 R12: ffff88807f7a0000
R13: dffffc0000000000 R14: ffff888021da1000 R15: ffff88807c10a110
FS:  0000000000000000(0000) GS:ffff8880b9400000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000563828985bd8 CR3: 000000002e0cc000 CR4: 00000000003506f0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
 <TASK>
 usb_stor_control_thread+0x4b1/0xa50 drivers/usb/storage/usb.c:368
 kthread+0x2ef/0x390 kernel/kthread.c:388
 ret_from_fork+0x4b/0x80 arch/x86/kernel/process.c:147
 ret_from_fork_asm+0x1b/0x30 arch/x86/entry/entry_64.S:242
 </TASK>
Modules linked in:
---[ end trace 0000000000000000 ]---
RIP: 0010:isd200_scsi_to_ata drivers/usb/storage/isd200.c:1318 [inline]
RIP: 0010:isd200_ata_command+0x776/0x2380 drivers/usb/storage/isd200.c:1529
Code: 62 fa 49 8d 7c 24 0c 48 89 f8 48 c1 e8 03 42 0f b6 04 28 84 c0 0f 85 00 18 00 00 41 0f b7 5c 24 0c 48 8b 7c 24 18 89 f8 31 d2 <f7> f3 41 89 d0 49 83 c4 06 4c 89 e0 48 c1 e8 03 42 0f b6 04 28 84
RSP: 0018:ffffc900043ffc00 EFLAGS: 00010246
RAX: 0000000000000000 RBX: 0000000000000000 RCX: ffff888023230000
RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000000
RBP: ffffc900043ffd50 R08: ffffffff873161fd R09: ffffffff87315c95
R10: 0000000000000008 R11: ffff888023230000 R12: ffff88807f7a0000
R13: dffffc0000000000 R14: ffff888021da1000 R15: ffff88807c10a110
FS:  0000000000000000(0000) GS:ffff8880b9400000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000563828985bd8 CR3: 000000000df32000 CR4: 00000000003506f0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
----------------
Code disassembly (best guess), 1 bytes skipped:
   0:	fa                   	cli
   1:	49 8d 7c 24 0c       	lea    0xc(%r12),%rdi
   6:	48 89 f8             	mov    %rdi,%rax
   9:	48 c1 e8 03          	shr    $0x3,%rax
   d:	42 0f b6 04 28       	movzbl (%rax,%r13,1),%eax
  12:	84 c0                	test   %al,%al
  14:	0f 85 00 18 00 00    	jne    0x181a
  1a:	41 0f b7 5c 24 0c    	movzwl 0xc(%r12),%ebx
  20:	48 8b 7c 24 18       	mov    0x18(%rsp),%rdi
  25:	89 f8                	mov    %edi,%eax
  27:	31 d2                	xor    %edx,%edx
* 29:	f7 f3                	div    %ebx <-- trapping instruction
  2b:	41 89 d0             	mov    %edx,%r8d
  2e:	49 83 c4 06          	add    $0x6,%r12
  32:	4c 89 e0             	mov    %r12,%rax
  35:	48 c1 e8 03          	shr    $0x3,%rax
  39:	42 0f b6 04 28       	movzbl (%rax,%r13,1),%eax
  3e:	84                   	.byte 0x84


---
This report is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzkaller@googlegroups.com.

syzbot will keep track of this issue. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
For information about bisection process see: https://goo.gl/tpsmEJ#bisection

If the report is already addressed, let syzbot know by replying with:
#syz fix: exact-commit-title

If you want syzbot to run the reproducer, reply with:
#syz test: git://repo/address.git branch-or-commit-hash
If you attach or paste a git patch, syzbot will apply it before testing.

If you want to overwrite report's subsystems, reply with:
#syz set subsystems: new-subsystem
(See the list of subsystem names on the web dashboard)

If the report is a duplicate of another one, reply with:
#syz dup: exact-subject-of-another-report

If you want to undo deduplication, reply with:
#syz undup

Re: [syzbot] [usb-storage?] divide error in isd200_ata_command

[Oliver Neukum]

Hi,

On 26.02.24 10:42, syzbot wrote:
> Hello,
> 
> syzbot found the following issue on:
> 
> HEAD commit:    f2e367d6ad3b Merge tag 'for-6.8/dm-fix-3' of git://git.ker..
> git tree:       upstream
> console+strace: https://syzkaller.appspot.com/x/log.txt?x=114e10e4180000
> kernel config:  https://syzkaller.appspot.com/x/.config?x=eff9f3183d0a20dd
> dashboard link: https://syzkaller.appspot.com/bug?extid=28748250ab47a8f04100
> compiler:       Debian clang version 15.0.6, GNU ld (GNU Binutils for Debian) 2.40
> syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=1064b372180000
> C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=10aca6ac180000
> 
> Downloadable assets:
> disk image: https://storage.googleapis.com/syzbot-assets/c55ca1fdc5ad/disk-f2e367d6.raw.xz
> vmlinux: https://storage.googleapis.com/syzbot-assets/4556a82fb4ed/vmlinux-f2e367d6.xz
> kernel image: https://storage.googleapis.com/syzbot-assets/95338ed9dad1/bzImage-f2e367d6.xz
> 
> The issue was bisected to:
> 
> commit 321da3dc1f3c92a12e3c5da934090d2992a8814c
> Author: Martin K. Petersen <martin.petersen@oracle.com>
> Date:   Tue Feb 13 14:33:06 2024 +0000
> 
>      scsi: sd: usb_storage: uas: Access media prior to querying device properties

preliminary analysis:

It oopses here:

		} else {
			if (!id[ATA_ID_SECTORS] || !id[ATA_ID_HEADS])
				goto too_early;
			sectnum = (u8)((lba % id[ATA_ID_SECTORS]) + 1);
			cylinder = (u16)(lba / (id[ATA_ID_SECTORS] *
					id[ATA_ID_HEADS]));

in isd200_scsi_to_ata() because it must not be called before isd200_get_inquiry_data()
has completed.

That raises two questions.

1) should we limit the read_before_ms flag to the cases transparent SCSI is used?
2) does isd200_get_inquiry_data() need to validate what it reads?

	Regards
		Oliver

Re: [syzbot] [usb-storage?] divide error in isd200_ata_command

[Alan Stern]

On Mon, Feb 26, 2024 at 11:59:06AM +0100, Oliver Neukum wrote:
> Hi,
> 
> On 26.02.24 10:42, syzbot wrote:
> > Hello,
> > 
> > syzbot found the following issue on:
> > 
> > HEAD commit:    f2e367d6ad3b Merge tag 'for-6.8/dm-fix-3' of git://git.ker..
> > git tree:       upstream
> > console+strace: https://syzkaller.appspot.com/x/log.txt?x=114e10e4180000
> > kernel config:  https://syzkaller.appspot.com/x/.config?x=eff9f3183d0a20dd
> > dashboard link: https://syzkaller.appspot.com/bug?extid=28748250ab47a8f04100
> > compiler:       Debian clang version 15.0.6, GNU ld (GNU Binutils for Debian) 2.40
> > syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=1064b372180000
> > C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=10aca6ac180000
> > 
> > Downloadable assets:
> > disk image: https://storage.googleapis.com/syzbot-assets/c55ca1fdc5ad/disk-f2e367d6.raw.xz
> > vmlinux: https://storage.googleapis.com/syzbot-assets/4556a82fb4ed/vmlinux-f2e367d6.xz
> > kernel image: https://storage.googleapis.com/syzbot-assets/95338ed9dad1/bzImage-f2e367d6.xz
> > 
> > The issue was bisected to:
> > 
> > commit 321da3dc1f3c92a12e3c5da934090d2992a8814c
> > Author: Martin K. Petersen <martin.petersen@oracle.com>
> > Date:   Tue Feb 13 14:33:06 2024 +0000
> > 
> >      scsi: sd: usb_storage: uas: Access media prior to querying device properties
> 
> preliminary analysis:
> 
> It oopses here:
> 
> 		} else {
> 			if (!id[ATA_ID_SECTORS] || !id[ATA_ID_HEADS])
> 				goto too_early;

Those two lines are debugging code you added, right?

> 			sectnum = (u8)((lba % id[ATA_ID_SECTORS]) + 1);
> 			cylinder = (u16)(lba / (id[ATA_ID_SECTORS] *
> 					id[ATA_ID_HEADS]));
> 
> in isd200_scsi_to_ata() because it must not be called before isd200_get_inquiry_data()
> has completed.

It can't be; isd200_get_inquiry_data is called by isd200_Initialization 
during probe before any SCSI commands are transmitted.

> That raises two questions.
> 
> 1) should we limit the read_before_ms flag to the cases transparent SCSI is used?

That won't help; the inquiry data will still be wrong.

> 2) does isd200_get_inquiry_data() need to validate what it reads?

Yes.  At least to make sure that we're not going to divide by 0.  This 
means that id[ATA_ID_SECTORS] and id[ATA_ID_HEADS] must both be > 0.  
Since they are 16-bit quantities, we don't have to worry about them 
overflowing.

Do you want to submit a fix for syzbot to test?

Alan Stern

Re: [syzbot] [usb-storage?] divide error in isd200_ata_command

[Martin K. Petersen]

Alan,

>> in isd200_scsi_to_ata() because it must not be called before
>> isd200_get_inquiry_data() has completed.
>
> It can't be; isd200_get_inquiry_data is called by
> isd200_Initialization during probe before any SCSI commands are
> transmitted.

How do we end up with bad inquiry data (or rather bad ATA ID data)?

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [syzbot] [usb-storage?] divide error in isd200_ata_command

[Alan Stern]

On Mon, Feb 26, 2024 at 09:46:39PM -0500, Martin K. Petersen wrote:
> 
> Alan,
> 
> >> in isd200_scsi_to_ata() because it must not be called before
> >> isd200_get_inquiry_data() has completed.
> >
> > It can't be; isd200_get_inquiry_data is called by
> > isd200_Initialization during probe before any SCSI commands are
> > transmitted.
> 
> How do we end up with bad inquiry data (or rather bad ATA ID data)?

The data doesn't come from an actual ISD200 device.  It's from a 
deliberately faulty (for the obvious reason) emulation in userspace, 
created by the syzbot test code.

Alan Stern

Re: [syzbot] [usb-storage?] divide error in isd200_ata_command

[Alan Stern]

On Mon, Feb 26, 2024 at 01:42:26AM -0800, syzbot wrote:
> Hello,
> 
> syzbot found the following issue on:
> 
> HEAD commit:    f2e367d6ad3b Merge tag 'for-6.8/dm-fix-3' of git://git.ker..
> git tree:       upstream
> console+strace: https://syzkaller.appspot.com/x/log.txt?x=114e10e4180000
> kernel config:  https://syzkaller.appspot.com/x/.config?x=eff9f3183d0a20dd
> dashboard link: https://syzkaller.appspot.com/bug?extid=28748250ab47a8f04100
> compiler:       Debian clang version 15.0.6, GNU ld (GNU Binutils for Debian) 2.40
> syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=1064b372180000
> C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=10aca6ac180000
> 
> Downloadable assets:
> disk image: https://storage.googleapis.com/syzbot-assets/c55ca1fdc5ad/disk-f2e367d6.raw.xz
> vmlinux: https://storage.googleapis.com/syzbot-assets/4556a82fb4ed/vmlinux-f2e367d6.xz
> kernel image: https://storage.googleapis.com/syzbot-assets/95338ed9dad1/bzImage-f2e367d6.xz
> 
> The issue was bisected to:
> 
> commit 321da3dc1f3c92a12e3c5da934090d2992a8814c
> Author: Martin K. Petersen <martin.petersen@oracle.com>
> Date:   Tue Feb 13 14:33:06 2024 +0000
> 
>     scsi: sd: usb_storage: uas: Access media prior to querying device properties
> 
> bisection log:  https://syzkaller.appspot.com/x/bisect.txt?x=15a3934a180000
> final oops:     https://syzkaller.appspot.com/x/report.txt?x=17a3934a180000
> console output: https://syzkaller.appspot.com/x/log.txt?x=13a3934a180000
> 
> IMPORTANT: if you fix the issue, please add the following tag to the commit:
> Reported-by: syzbot+28748250ab47a8f04100@syzkaller.appspotmail.com
> Fixes: 321da3dc1f3c ("scsi: sd: usb_storage: uas: Access media prior to querying device properties")
> 
> divide error: 0000 [#1] PREEMPT SMP KASAN PTI
> CPU: 0 PID: 5070 Comm: usb-storage Not tainted 6.8.0-rc5-syzkaller-00297-gf2e367d6ad3b #0
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/25/2024
> RIP: 0010:isd200_scsi_to_ata drivers/usb/storage/isd200.c:1318 [inline]
> RIP: 0010:isd200_ata_command+0x776/0x2380 drivers/usb/storage/isd200.c:1529

#syz test: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/ f2e367d6ad3b

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;
 }
 
 

Re: [syzbot] [usb-storage?] divide error in isd200_ata_command

[syzbot]

Hello,

syzbot tried to test the proposed patch but the build/boot failed:

failed to checkout kernel repo https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/ on commit f2e367d6ad3b: failed to run ["git" "fetch" "--force" "--tags" "7b440d1b40dd93ea98b5af6bba55ffca63425216" "f2e367d6ad3b"]: exit status 128
fatal: couldn't find remote ref f2e367d6ad3b



Tested on:

commit:         [unknown 
git tree:       https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/ f2e367d6ad3b
kernel config:  https://syzkaller.appspot.com/x/.config?x=eff9f3183d0a20dd
dashboard link: https://syzkaller.appspot.com/bug?extid=28748250ab47a8f04100
compiler:       
patch:          https://syzkaller.appspot.com/x/patch.diff?x=14296a74180000

Re: [syzbot] [usb-storage?] divide error in isd200_ata_command

[Alan Stern]

On Tue, Feb 27, 2024 at 09:20:03PM -0800, syzbot wrote:
> Hello,
> 
> syzbot tried to test the proposed patch but the build/boot failed:
> 
> failed to checkout kernel repo https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/ on commit f2e367d6ad3b: failed to run ["git" "fetch" "--force" "--tags" "7b440d1b40dd93ea98b5af6bba55ffca63425216" "f2e367d6ad3b"]: exit status 128
> fatal: couldn't find remote ref f2e367d6ad3b

I'm going to guess this was a temporary failure and try again.  If that 
wasn't the case, something is seriously wrong somewhere.  I had no 
trouble accessing that commit using the git.kernel.org web interface.

Alan Stern

On Mon, Feb 26, 2024 at 01:42:26AM -0800, syzbot wrote:
> Hello,
> 
> syzbot found the following issue on:
> 
> HEAD commit:    f2e367d6ad3b Merge tag 'for-6.8/dm-fix-3' of git://git.ker..
> git tree:       upstream

#syz test: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/ f2e367d6ad3b

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;
 }
 
 

Re: [syzbot] [usb-storage?] divide error in isd200_ata_command

[syzbot]

Hello,

syzbot tried to test the proposed patch but the build/boot failed:

failed to checkout kernel repo https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/ on commit f2e367d6ad3b: failed to run ["git" "fetch" "--force" "--tags" "7b440d1b40dd93ea98b5af6bba55ffca63425216" "f2e367d6ad3b"]: exit status 128
fatal: couldn't find remote ref f2e367d6ad3b



Tested on:

commit:         [unknown 
git tree:       https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/ f2e367d6ad3b
kernel config:  https://syzkaller.appspot.com/x/.config?x=eff9f3183d0a20dd
dashboard link: https://syzkaller.appspot.com/bug?extid=28748250ab47a8f04100
compiler:       
patch:          https://syzkaller.appspot.com/x/patch.diff?x=164aa516180000

Re: [syzbot] [usb-storage?] divide error in isd200_ata_command

[Oliver Neukum]

On 26.02.24 19:13, Alan Stern wrote:

>> It oopses here:
>>
>> 		} else {
>> 			if (!id[ATA_ID_SECTORS] || !id[ATA_ID_HEADS])
>> 				goto too_early;
> 
> Those two lines are debugging code you added, right?

Yes, sorry about that.

> 
>> 			sectnum = (u8)((lba % id[ATA_ID_SECTORS]) + 1);
>> 			cylinder = (u16)(lba / (id[ATA_ID_SECTORS] *
>> 					id[ATA_ID_HEADS]));
>>
>> in isd200_scsi_to_ata() because it must not be called before isd200_get_inquiry_data()
>> has completed.
> 
> It can't be; isd200_get_inquiry_data is called by isd200_Initialization
> during probe before any SCSI commands are transmitted.

So, you are concluding that the bisection is spurious because
without that patch the SCSI layer would see a capacity of zero
and not even try to read anything?

	Regards
		Oliver

Re: [syzbot] [usb-storage?] divide error in isd200_ata_command

[Aleksandr Nogikh]

Hi Alan,

Please try it once more with the full commit hash.

-- 
Aleksandr

On Wed, Feb 28, 2024 at 5:12 PM Alan Stern <stern@rowland.harvard.edu> wrote:
>
> On Tue, Feb 27, 2024 at 09:20:03PM -0800, syzbot wrote:
> > Hello,
> >
> > syzbot tried to test the proposed patch but the build/boot failed:
> >
> > failed to checkout kernel repo https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/ on commit f2e367d6ad3b: failed to run ["git" "fetch" "--force" "--tags" "7b440d1b40dd93ea98b5af6bba55ffca63425216" "f2e367d6ad3b"]: exit status 128
> > fatal: couldn't find remote ref f2e367d6ad3b
>
> I'm going to guess this was a temporary failure and try again.  If that
> wasn't the case, something is seriously wrong somewhere.  I had no
> trouble accessing that commit using the git.kernel.org web interface.
>
> Alan Stern
>
> On Mon, Feb 26, 2024 at 01:42:26AM -0800, syzbot wrote:
> > Hello,
> >
> > syzbot found the following issue on:
> >
> > HEAD commit:    f2e367d6ad3b Merge tag 'for-6.8/dm-fix-3' of git://git.ker..
> > git tree:       upstream
>
> #syz test: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/ f2e367d6ad3b
>
> 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;
>  }
>
>
>

Re: [syzbot] [usb-storage?] divide error in isd200_ata_command

[Alan Stern]

On Wed, Feb 28, 2024 at 05:52:50PM +0100, Aleksandr Nogikh wrote:
> Hi Alan,
> 
> Please try it once more with the full commit hash.

Thanks for the advice.  Are you a good person to complain to about the 
difference between what syzbot provides and what it will accept?  This 
bug report states

HEAD commit:    f2e367d6ad3b Merge tag 'for-6.8/dm-fix-3' of git://git.ker..
git tree:       upstream

But if I specify "upstream" as the git tree on a syz test request, it 
doesn't accept it.  Now you're suggesting that if I put f2e367d6ad3b as 
the commit ID, it won't accept it.

There's probably already a bugfix request for this, but I'd like to push 
on it some more.  Syzbot's output should be acceptable as its input!

Okay, here goes with the full commit ID...

Alan Stern

On Mon, Feb 26, 2024 at 01:42:26AM -0800, syzbot wrote:
> Hello,
> 
> syzbot found the following issue on:
> 
> HEAD commit:    f2e367d6ad3b Merge tag 'for-6.8/dm-fix-3' of git://git.ker..
> git tree:       upstream
> console+strace: https://syzkaller.appspot.com/x/log.txt?x=114e10e4180000
> kernel config:  https://syzkaller.appspot.com/x/.config?x=eff9f3183d0a20dd
> dashboard link: https://syzkaller.appspot.com/bug?extid=28748250ab47a8f04100
> compiler:       Debian clang version 15.0.6, GNU ld (GNU Binutils for Debian) 2.40
> syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=1064b372180000
> C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=10aca6ac180000
> 
> Downloadable assets:
> disk image: https://storage.googleapis.com/syzbot-assets/c55ca1fdc5ad/disk-f2e367d6.raw.xz
> vmlinux: https://storage.googleapis.com/syzbot-assets/4556a82fb4ed/vmlinux-f2e367d6.xz
> kernel image: https://storage.googleapis.com/syzbot-assets/95338ed9dad1/bzImage-f2e367d6.xz
> 
> The issue was bisected to:
> 
> commit 321da3dc1f3c92a12e3c5da934090d2992a8814c
> Author: Martin K. Petersen <martin.petersen@oracle.com>
> Date:   Tue Feb 13 14:33:06 2024 +0000
> 
>     scsi: sd: usb_storage: uas: Access media prior to querying device properties
> 
> bisection log:  https://syzkaller.appspot.com/x/bisect.txt?x=15a3934a180000
> final oops:     https://syzkaller.appspot.com/x/report.txt?x=17a3934a180000
> console output: https://syzkaller.appspot.com/x/log.txt?x=13a3934a180000
> 
> IMPORTANT: if you fix the issue, please add the following tag to the commit:
> Reported-by: syzbot+28748250ab47a8f04100@syzkaller.appspotmail.com
> Fixes: 321da3dc1f3c ("scsi: sd: usb_storage: uas: Access media prior to querying device properties")
> 
> divide error: 0000 [#1] PREEMPT SMP KASAN PTI
> CPU: 0 PID: 5070 Comm: usb-storage Not tainted 6.8.0-rc5-syzkaller-00297-gf2e367d6ad3b #0
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/25/2024
> RIP: 0010:isd200_scsi_to_ata drivers/usb/storage/isd200.c:1318 [inline]
> RIP: 0010:isd200_ata_command+0x776/0x2380 drivers/usb/storage/isd200.c:1529

#syz test: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/ f2e367d6ad3bdc527c2b14e759c2f010d6b2b7a1
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;
 }
 
 

Re: [syzbot] [usb-storage?] divide error in isd200_ata_command

[Alan Stern]

On Wed, Feb 28, 2024 at 05:20:29PM +0100, Oliver Neukum wrote:
> On 26.02.24 19:13, Alan Stern wrote:
> 
> > > It oopses here:
> > > 
> > > 		} else {
> > > 			if (!id[ATA_ID_SECTORS] || !id[ATA_ID_HEADS])
> > > 				goto too_early;
> > 
> > Those two lines are debugging code you added, right?
> 
> Yes, sorry about that.
> 
> > 
> > > 			sectnum = (u8)((lba % id[ATA_ID_SECTORS]) + 1);
> > > 			cylinder = (u16)(lba / (id[ATA_ID_SECTORS] *
> > > 					id[ATA_ID_HEADS]));
> > > 
> > > in isd200_scsi_to_ata() because it must not be called before isd200_get_inquiry_data()
> > > has completed.
> > 
> > It can't be; isd200_get_inquiry_data is called by isd200_Initialization
> > during probe before any SCSI commands are transmitted.
> 
> So, you are concluding that the bisection is spurious because
> without that patch the SCSI layer would see a capacity of zero
> and not even try to read anything?

I don't know.  My guess is that without this patch, the test would fail 
for some reason before the SCSI layer has a chance to issue a READ command.  
Maybe because of a zero capacity, like you said, or maybe something 
else.

Whatever the reason, it looks like Martin's commit merely exposed a 
problem which has existed all along.

Alan Stern

Re: [syzbot] [usb-storage?] divide error in isd200_ata_command

[syzbot]

Hello,

syzbot has tested the proposed patch and the reproducer did not trigger any issue:

Reported-and-tested-by: syzbot+28748250ab47a8f04100@syzkaller.appspotmail.com

Tested on:

commit:         f2e367d6 Merge tag 'for-6.8/dm-fix-3' of git://git.ker..
git tree:       https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/
console output: https://syzkaller.appspot.com/x/log.txt?x=15da3dba180000
kernel config:  https://syzkaller.appspot.com/x/.config?x=eff9f3183d0a20dd
dashboard link: https://syzkaller.appspot.com/bug?extid=28748250ab47a8f04100
compiler:       Debian clang version 15.0.6, GNU ld (GNU Binutils for Debian) 2.40
patch:          https://syzkaller.appspot.com/x/patch.diff?x=13287836180000

Note: testing is done by a robot and is best-effort only.

Re: [syzbot] [usb-storage?] divide error in isd200_ata_command

[Aleksandr Nogikh]

On Wed, Feb 28, 2024 at 8:18 PM Alan Stern <stern@rowland.harvard.edu> wrote:
>
> On Wed, Feb 28, 2024 at 05:52:50PM +0100, Aleksandr Nogikh wrote:
> > Hi Alan,
> >
> > Please try it once more with the full commit hash.
>
> Thanks for the advice.  Are you a good person to complain to about the
> difference between what syzbot provides and what it will accept?  This
> bug report states
>
> HEAD commit:    f2e367d6ad3b Merge tag 'for-6.8/dm-fix-3' of git://git.ker..
> git tree:       upstream
>
> But if I specify "upstream" as the git tree on a syz test request, it
> doesn't accept it.  Now you're suggesting that if I put f2e367d6ad3b as
> the commit ID, it won't accept it.
>
> There's probably already a bugfix request for this, but I'd like to push
> on it some more.  Syzbot's output should be acceptable as its input!

That all totally makes sense. Thanks for highlighting the problems!

For accepting "upstream" (and alike) as input, there was already a github issue:
https://github.com/google/syzkaller/issues/2265
That syzbot is not able to fetch commits by their short hashes was
only discovered yesterday.

I've just sent PRs with fixes for both issues.

If there's anything else that can make syzbot reports better, please
let me know :)

-- 
Aleksandr

>
> Okay, here goes with the full commit ID...
>
> Alan Stern
>
> On Mon, Feb 26, 2024 at 01:42:26AM -0800, syzbot wrote:
> > Hello,
> >
> > syzbot found the following issue on:
> >
> > HEAD commit:    f2e367d6ad3b Merge tag 'for-6.8/dm-fix-3' of git://git.ker..
> > git tree:       upstream
> > console+strace: https://syzkaller.appspot.com/x/log.txt?x=114e10e4180000
> > kernel config:  https://syzkaller.appspot.com/x/.config?x=eff9f3183d0a20dd
> > dashboard link: https://syzkaller.appspot.com/bug?extid=28748250ab47a8f04100
> > compiler:       Debian clang version 15.0.6, GNU ld (GNU Binutils for Debian) 2.40
> > syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=1064b372180000
> > C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=10aca6ac180000
> >
> > Downloadable assets:
> > disk image: https://storage.googleapis.com/syzbot-assets/c55ca1fdc5ad/disk-f2e367d6.raw.xz
> > vmlinux: https://storage.googleapis.com/syzbot-assets/4556a82fb4ed/vmlinux-f2e367d6.xz
> > kernel image: https://storage.googleapis.com/syzbot-assets/95338ed9dad1/bzImage-f2e367d6.xz
> >
> > The issue was bisected to:
> >
> > commit 321da3dc1f3c92a12e3c5da934090d2992a8814c
> > Author: Martin K. Petersen <martin.petersen@oracle.com>
> > Date:   Tue Feb 13 14:33:06 2024 +0000
> >
> >     scsi: sd: usb_storage: uas: Access media prior to querying device properties
> >
> > bisection log:  https://syzkaller.appspot.com/x/bisect.txt?x=15a3934a180000
> > final oops:     https://syzkaller.appspot.com/x/report.txt?x=17a3934a180000
> > console output: https://syzkaller.appspot.com/x/log.txt?x=13a3934a180000
> >
> > IMPORTANT: if you fix the issue, please add the following tag to the commit:
> > Reported-by: syzbot+28748250ab47a8f04100@syzkaller.appspotmail.com
> > Fixes: 321da3dc1f3c ("scsi: sd: usb_storage: uas: Access media prior to querying device properties")
> >
> > divide error: 0000 [#1] PREEMPT SMP KASAN PTI
> > CPU: 0 PID: 5070 Comm: usb-storage Not tainted 6.8.0-rc5-syzkaller-00297-gf2e367d6ad3b #0
> > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/25/2024
> > RIP: 0010:isd200_scsi_to_ata drivers/usb/storage/isd200.c:1318 [inline]
> > RIP: 0010:isd200_ata_command+0x776/0x2380 drivers/usb/storage/isd200.c:1529
>
> #syz test: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/ f2e367d6ad3bdc527c2b14e759c2f010d6b2b7a1
> 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;
>  }
>
>
>
>

Re: [syzbot] [usb-storage?] divide error in isd200_ata_command

[Alan Stern]

On Thu, Feb 29, 2024 at 04:40:05PM +0100, Aleksandr Nogikh wrote:
> On Wed, Feb 28, 2024 at 8:18 PM Alan Stern <stern@rowland.harvard.edu> wrote:
> >
> > On Wed, Feb 28, 2024 at 05:52:50PM +0100, Aleksandr Nogikh wrote:
> > > Hi Alan,
> > >
> > > Please try it once more with the full commit hash.
> >
> > Thanks for the advice.  Are you a good person to complain to about the
> > difference between what syzbot provides and what it will accept?  This
> > bug report states
> >
> > HEAD commit:    f2e367d6ad3b Merge tag 'for-6.8/dm-fix-3' of git://git.ker..
> > git tree:       upstream
> >
> > But if I specify "upstream" as the git tree on a syz test request, it
> > doesn't accept it.  Now you're suggesting that if I put f2e367d6ad3b as
> > the commit ID, it won't accept it.
> >
> > There's probably already a bugfix request for this, but I'd like to push
> > on it some more.  Syzbot's output should be acceptable as its input!
> 
> That all totally makes sense. Thanks for highlighting the problems!
> 
> For accepting "upstream" (and alike) as input, there was already a github issue:
> https://github.com/google/syzkaller/issues/2265
> That syzbot is not able to fetch commits by their short hashes was
> only discovered yesterday.
> 
> I've just sent PRs with fixes for both issues.
> 
> If there's anything else that can make syzbot reports better, please
> let me know :)

That's great news!  Thanks a lot.

How will we know when the fixes have been accepted and we can use them?

Alan Stern

Re: [syzbot] [usb-storage?] divide error in isd200_ata_command

[Aleksandr Nogikh]

On Thu, Feb 29, 2024 at 5:12 PM Alan Stern <stern@rowland.harvard.edu> wrote:
>
> On Thu, Feb 29, 2024 at 04:40:05PM +0100, Aleksandr Nogikh wrote:
> > On Wed, Feb 28, 2024 at 8:18 PM Alan Stern <stern@rowland.harvard.edu> wrote:
> > >
> > > On Wed, Feb 28, 2024 at 05:52:50PM +0100, Aleksandr Nogikh wrote:
> > > > Hi Alan,
> > > >
> > > > Please try it once more with the full commit hash.
> > >
> > > Thanks for the advice.  Are you a good person to complain to about the
> > > difference between what syzbot provides and what it will accept?  This
> > > bug report states
> > >
> > > HEAD commit:    f2e367d6ad3b Merge tag 'for-6.8/dm-fix-3' of git://git.ker..
> > > git tree:       upstream
> > >
> > > But if I specify "upstream" as the git tree on a syz test request, it
> > > doesn't accept it.  Now you're suggesting that if I put f2e367d6ad3b as
> > > the commit ID, it won't accept it.
> > >
> > > There's probably already a bugfix request for this, but I'd like to push
> > > on it some more.  Syzbot's output should be acceptable as its input!
> >
> > That all totally makes sense. Thanks for highlighting the problems!
> >
> > For accepting "upstream" (and alike) as input, there was already a github issue:
> > https://github.com/google/syzkaller/issues/2265
> > That syzbot is not able to fetch commits by their short hashes was
> > only discovered yesterday.
> >
> > I've just sent PRs with fixes for both issues.
> >
> > If there's anything else that can make syzbot reports better, please
> > let me know :)
>
> That's great news!  Thanks a lot.
>
> How will we know when the fixes have been accepted and we can use them?

The features will become available several hours after these PRs are
approved and merged:
https://github.com/google/syzkaller/pull/4538
https://github.com/google/syzkaller/pull/4539

I think Tuesday or Wednesday next week are reasonable estimates :)

-- 
Aleksandr

>
> Alan Stern

[PATCH] USB: usb-storage: Prevent divide-by-0 error in isd200_ata_command

[Alan Stern]

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;
 }
 
 

Re: [PATCH] USB: usb-storage: Prevent divide-by-0 error in isd200_ata_command

[Martin K. Petersen]

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>

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH] USB: usb-storage: Prevent divide-by-0 error in isd200_ata_command

[PrasannaKumar Muralidharan]

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