| Message ID | 20190123191237.119996-1-bvanassche@acm.org (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | db5db4b91cabcf57f3efd98d92d24ab875cde8ae |
| Headers | show |
| Series | sd: Protect against submitting READ(6) or WRITE(6) with 256 logical blocks | expand |
On 2019-01-23 2:12 p.m., Bart Van Assche wrote: > Since the READ(6) and WRITE(6) commands interpret a zero in the transfer > length field in the CDB as 256 logical blocks, avoid submitting such > commands. > > Cc: Douglas Gilbert <dgilbert@interlog.com> > Cc: Hannes Reinecke <hare@suse.com> > Cc: Christoph Hellwig <hch@lst.de> > Reported-by: Douglas Gilbert <dgilbert@interlog.com> > Signed-off-by: Bart Van Assche <bvanassche@acm.org> Reviewed-by: Douglas Gilbert <dgilbert@interlog.com> > --- > drivers/scsi/sd.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c > index 4e69f182a1e5..b0eb83526c54 100644 > --- a/drivers/scsi/sd.c > +++ b/drivers/scsi/sd.c > @@ -1129,6 +1129,10 @@ static blk_status_t sd_setup_rw6_cmnd(struct scsi_cmnd *cmd, bool write, > sector_t lba, unsigned int nr_blocks, > unsigned char flags) > { > + /* Avoid that 0 blocks gets translated into 256 blocks. */ > + if (WARN_ON_ONCE(nr_blocks == 0)) > + return BLK_STS_IOERR; > + > if (unlikely(flags & 0x8)) { > /* > * This happens only if this drive failed 10byte rw >
On 1/23/19 8:12 PM, Bart Van Assche wrote: > Since the READ(6) and WRITE(6) commands interpret a zero in the transfer > length field in the CDB as 256 logical blocks, avoid submitting such > commands. > > Cc: Douglas Gilbert <dgilbert@interlog.com> > Cc: Hannes Reinecke <hare@suse.com> > Cc: Christoph Hellwig <hch@lst.de> > Reported-by: Douglas Gilbert <dgilbert@interlog.com> > Signed-off-by: Bart Van Assche <bvanassche@acm.org> > --- > drivers/scsi/sd.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c > index 4e69f182a1e5..b0eb83526c54 100644 > --- a/drivers/scsi/sd.c > +++ b/drivers/scsi/sd.c > @@ -1129,6 +1129,10 @@ static blk_status_t sd_setup_rw6_cmnd(struct scsi_cmnd *cmd, bool write, > sector_t lba, unsigned int nr_blocks, > unsigned char flags) > { > + /* Avoid that 0 blocks gets translated into 256 blocks. */ > + if (WARN_ON_ONCE(nr_blocks == 0)) > + return BLK_STS_IOERR; > + > if (unlikely(flags & 0x8)) { > /* > * This happens only if this drive failed 10byte rw > Reviewed-by: Hannes Reinecke <hare@suse.com> Cheers, Hannes
Bart, > Since the READ(6) and WRITE(6) commands interpret a zero in the transfer > length field in the CDB as 256 logical blocks, avoid submitting such > commands. Applied to 5.1/scsi-queue, thanks.
On Wed, Jan 23, 2019 at 11:12:37AM -0800, Bart Van Assche wrote: > diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c > index 4e69f182a1e5..b0eb83526c54 100644 > --- a/drivers/scsi/sd.c > +++ b/drivers/scsi/sd.c > @@ -1129,6 +1129,10 @@ static blk_status_t sd_setup_rw6_cmnd(struct scsi_cmnd *cmd, bool write, > sector_t lba, unsigned int nr_blocks, > unsigned char flags) > { > + /* Avoid that 0 blocks gets translated into 256 blocks. */ > + if (WARN_ON_ONCE(nr_blocks == 0)) > + return BLK_STS_IOERR; > + While the WARN_ON here looks helpful shouldn't we instead ensure that sd_setup_rw6_cmnd never gets called with a 0 argument instead of bailing out like this?
On 1/29/19 12:40 AM, Christoph Hellwig wrote: > On Wed, Jan 23, 2019 at 11:12:37AM -0800, Bart Van Assche wrote: >> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c >> index 4e69f182a1e5..b0eb83526c54 100644 >> --- a/drivers/scsi/sd.c >> +++ b/drivers/scsi/sd.c >> @@ -1129,6 +1129,10 @@ static blk_status_t sd_setup_rw6_cmnd(struct scsi_cmnd *cmd, bool write, >> sector_t lba, unsigned int nr_blocks, >> unsigned char flags) >> { >> + /* Avoid that 0 blocks gets translated into 256 blocks. */ >> + if (WARN_ON_ONCE(nr_blocks == 0)) >> + return BLK_STS_IOERR; >> + > > While the WARN_ON here looks helpful shouldn't we instead ensure that > sd_setup_rw6_cmnd never gets called with a 0 argument instead of bailing > out like this? Hi Christoph, Before I posted this patch I searched for code that submits read or write requests with length 0 but I haven't found any. do_iter_read() and do_iter_write() in fs/read_write.c do not submit any block layer requests if tot_len == 0. Are you perhaps aware of kernel code that can submit zero-length read or write requests? Thanks, Bart.
Bart, >> While the WARN_ON here looks helpful shouldn't we instead ensure that >> sd_setup_rw6_cmnd never gets called with a 0 argument instead of bailing >> out like this? > > Before I posted this patch I searched for code that submits read or > write requests with length 0 but I haven't found any. do_iter_read() > and do_iter_write() in fs/read_write.c do not submit any block layer > requests if tot_len == 0. Are you perhaps aware of kernel code that > can submit zero-length read or write requests? At some level I would have liked the sanity check in the top rw prep command. However, given that a zero transfer length is mostly a hypothetical scenario I tend to concur with sticking the check in rw6 outside of the hot path.
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c index 4e69f182a1e5..b0eb83526c54 100644 --- a/drivers/scsi/sd.c +++ b/drivers/scsi/sd.c @@ -1129,6 +1129,10 @@ static blk_status_t sd_setup_rw6_cmnd(struct scsi_cmnd *cmd, bool write, sector_t lba, unsigned int nr_blocks, unsigned char flags) { + /* Avoid that 0 blocks gets translated into 256 blocks. */ + if (WARN_ON_ONCE(nr_blocks == 0)) + return BLK_STS_IOERR; + if (unlikely(flags & 0x8)) { /* * This happens only if this drive failed 10byte rw
Since the READ(6) and WRITE(6) commands interpret a zero in the transfer length field in the CDB as 256 logical blocks, avoid submitting such commands. Cc: Douglas Gilbert <dgilbert@interlog.com> Cc: Hannes Reinecke <hare@suse.com> Cc: Christoph Hellwig <hch@lst.de> Reported-by: Douglas Gilbert <dgilbert@interlog.com> Signed-off-by: Bart Van Assche <bvanassche@acm.org> --- drivers/scsi/sd.c | 4 ++++ 1 file changed, 4 insertions(+)