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