| Message ID | 20210506073610.33867-1-phil@philpotter.co.uk (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | scsi: sd: skip checks when media is present if sd_read_capacity reports zero | expand |
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?
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
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).
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
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(+)