[PATCH] scsi: sd: skip checks when media is present if sd_read_capacity reports zero

[PATCH] scsi: sd: skip checks when media is present if sd_read_capacity reports zero

[Phillip Potter]

In sd_revalidate_disk, if sdkp->media_present is set, then sdkp->capacity
should not be zero. Therefore, jump to end of if block and skip remaining
checks/calls. Fixes a KMSAN-found uninit-value bug reported by syzbot at:
https://syzkaller.appspot.com/bug?id=197c8a3a2de61720a9b500ad485a7aba0065c6af

Reported-by: syzbot+6b02c1da3865f750164a@syzkaller.appspotmail.com
Signed-off-by: Phillip Potter <phil@philpotter.co.uk>
---
 drivers/scsi/sd.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index ed0b1bb99f08..a7fe53f492ae 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -3201,6 +3201,8 @@ static int sd_revalidate_disk(struct gendisk *disk)
 	 */
 	if (sdkp->media_present) {
 		sd_read_capacity(sdkp, buffer);
+		if (!sdkp->capacity)
+			goto present_block_end;
 
 		/*
 		 * set the default to rotational.  All non-rotational devices
@@ -3226,6 +3228,7 @@ static int sd_revalidate_disk(struct gendisk *disk)
 		sd_read_write_same(sdkp, buffer);
 		sd_read_security(sdkp, buffer);
 	}
+present_block_end:
 
 	/*
 	 * We now have all cache related info, determine how we deal
-- 
2.30.2

Re: [PATCH] scsi: sd: skip checks when media is present if sd_read_capacity reports zero

[Martin K. Petersen]

Hello Phillip!

> In sd_revalidate_disk, if sdkp->media_present is set, then sdkp->capacity
> should not be zero. Therefore, jump to end of if block and skip remaining
> checks/calls. Fixes a KMSAN-found uninit-value bug reported by syzbot at:
> https://syzkaller.appspot.com/bug?id=197c8a3a2de61720a9b500ad485a7aba0065c6af

The reported read of an uninitialized value is in scsi_mode_sense()
while inspecting a buffer returned from sending a MODE SENSE command to
the device. The buffer in question is memset() before executing the MODE
SENSE command. And we only look at the buffer contents if the MODE SENSE
operation was successful.

As far as I can tell the only way to end up reading uninitialized data
is if the device successfully completes the command but fails to
transfer the data buffer.

But maybe I'm missing something?

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH] scsi: sd: skip checks when media is present if sd_read_capacity reports zero

[Phillip Potter]

On Fri, May 21, 2021 at 04:00:10PM -0400, Martin K. Petersen wrote:
>
> Hello Phillip!
>
> > In sd_revalidate_disk, if sdkp->media_present is set, then sdkp->capacity
> > should not be zero. Therefore, jump to end of if block and skip remaining
> > checks/calls. Fixes a KMSAN-found uninit-value bug reported by syzbot at:
> > https://syzkaller.appspot.com/bug?id=197c8a3a2de61720a9b500ad485a7aba0065c6af
>
> The reported read of an uninitialized value is in scsi_mode_sense()
> while inspecting a buffer returned from sending a MODE SENSE command to
> the device. The buffer in question is memset() before executing the MODE
> SENSE command. And we only look at the buffer contents if the MODE SENSE
> operation was successful.
>
> As far as I can tell the only way to end up reading uninitialized data
> is if the device successfully completes the command but fails to
> transfer the data buffer.
>
> But maybe I'm missing something?
>
> --
> Martin K. Petersen    Oracle Linux Engineering

Dear Martin,

Thank you for your feedback firstly, much appreciated.

I may be misunderstanding this issue, but in my mind, if this issue is
possible to
trigger with a reproducer, then uninitialised data is being read? It
occurred to me
that a capacity of zero for a media which is present would make the following
function calls/checks invalid, hence the motivation for my patch, as
skipping all
those checks with such a size prevents this bug.

Another thing I noticed was that (unless I'm reading this wrong which
is certainly
possible) the buffer is never fully memset. It is allocated to be 512
bytes in size
(as SD_BUF_SIZE) and yet sd_do_mode_sense/scsi_mode_sense is never called
with a len param of this size but in fact much lower. Perhaps you're
right though and
my patch is not required? Certainly many KMSAN bugs are probably in areas where
logic is not affected by the uninitialised access.

Regards,
Phil

Re: [PATCH] scsi: sd: skip checks when media is present if sd_read_capacity reports zero

[Martin K. Petersen]

Phillip,

> It occurred to me that a capacity of zero for a media which is present
> would make the following function calls/checks invalid, hence the
> motivation for my patch, as skipping all those checks with such a size
> prevents this bug.

Yes and no. In theory most of these are orthogonal to what the reported
capacity is. But obviously the values reported are not terribly useful
if capacity is zero.

> Another thing I noticed was that (unless I'm reading this wrong which
> is certainly possible) the buffer is never fully memset. It is
> allocated to be 512 bytes in size (as SD_BUF_SIZE) and yet
> sd_do_mode_sense/scsi_mode_sense is never called with a len param of
> this size but in fact much lower.

Correct. To avoid a bazillion different allocations in the device
discovery path we allocate one buffer that the various functions then
take turns using. With suitable lengths passed in based on what the
protocol defines. I don't know if KMSAN is smart enough to handle that
scenario?

If it is then there must be a real problem lurking somewhere in the
discovery path. I would prefer to track that down (as opposed to masking
the problem by exiting early if capacity is zero).

-- 
Martin K. Petersen	Oracle Linux Engineering