| Message ID | 20210819073750.132601-1-damien.lemoal@wdc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | scsi: fix scsi_mode_sense() | expand |
On Thu, Aug 19, 2021 at 04:37:50PM +0900, Damien Le Moal wrote: > The allocation length field of the MODE SENSE 10 command is 16-bits, > occupying bytes 7 and 8 of the CDB. With this command, access to mode > pages larger than 255 bytes is thus possible. Make sure that > scsi_mode_sense() code reflects this by initializing the allocation > length field using put_unaligned_be16() instead of directly setting > only byte 8 of the CDB with the buffer length. > > While at it, also fix the folowing: > * use get_unaligned_be16() to retrieve the mode data length and block > descriptor length fields of the mode sense reply header instead of > using an open coded calculation. > * Fix the kdoc dbd argument explanation: the DBD bit stands for > Disable Block Descriptor, which is the opposite of what the dbd > argument description was. > > Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com> > --- > drivers/scsi/scsi_lib.c | 9 ++++----- > 1 file changed, 4 insertions(+), 5 deletions(-) > > diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c > index 7456a26aef51..92db00d81733 100644 > --- a/drivers/scsi/scsi_lib.c > +++ b/drivers/scsi/scsi_lib.c > @@ -2070,7 +2070,7 @@ EXPORT_SYMBOL_GPL(scsi_mode_select); > /** > * scsi_mode_sense - issue a mode sense, falling back from 10 to six bytes if necessary. > * @sdev: SCSI device to be queried > - * @dbd: set if mode sense will allow block descriptors to be returned > + * @dbd: set to prevent mode sense from returning block descriptors > * @modepage: mode page being requested > * @buffer: request buffer (may not be smaller than eight bytes) > * @len: length of request buffer. > @@ -2112,7 +2112,7 @@ scsi_mode_sense(struct scsi_device *sdev, int dbd, int modepage, > len = 8; > > cmd[0] = MODE_SENSE_10; > - cmd[8] = len; > + put_unaligned_be16(len, &cmd[7]); > header_length = 8; > } else { > if (len < 4) > @@ -2166,12 +2166,11 @@ scsi_mode_sense(struct scsi_device *sdev, int dbd, int modepage, > data->longlba = 0; > data->block_descriptor_length = 0; > } else if (use_10_for_ms) { > - data->length = buffer[0]*256 + buffer[1] + 2; > + data->length = get_unaligned_be16(&buffer[0]) + 2; > data->medium_type = buffer[2]; > data->device_specific = buffer[3]; > data->longlba = buffer[4] & 0x01; > - data->block_descriptor_length = buffer[6]*256 > - + buffer[7]; > + data->block_descriptor_length = get_unaligned_be16(&buffer[6]); > } else { > data->length = buffer[0] + 1; > data->medium_type = buffer[1]; (Nit: When the subject contains "fix", I think that people automatically look for a Fixes-tag. However, AFAIK, there isn't a caller that sends in a length > 1 byte. So a slightly better subject might have been: "scsi: allow scsi_mode_sense() to read more than 255 bytes") Reviewed-by: Niklas Cassel <niklas.cassel@wdc.com>
On 2021/08/19 18:30, Niklas Cassel wrote: > On Thu, Aug 19, 2021 at 04:37:50PM +0900, Damien Le Moal wrote: >> The allocation length field of the MODE SENSE 10 command is 16-bits, >> occupying bytes 7 and 8 of the CDB. With this command, access to mode >> pages larger than 255 bytes is thus possible. Make sure that >> scsi_mode_sense() code reflects this by initializing the allocation >> length field using put_unaligned_be16() instead of directly setting >> only byte 8 of the CDB with the buffer length. >> >> While at it, also fix the folowing: >> * use get_unaligned_be16() to retrieve the mode data length and block >> descriptor length fields of the mode sense reply header instead of >> using an open coded calculation. >> * Fix the kdoc dbd argument explanation: the DBD bit stands for >> Disable Block Descriptor, which is the opposite of what the dbd >> argument description was. >> >> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com> >> --- >> drivers/scsi/scsi_lib.c | 9 ++++----- >> 1 file changed, 4 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c >> index 7456a26aef51..92db00d81733 100644 >> --- a/drivers/scsi/scsi_lib.c >> +++ b/drivers/scsi/scsi_lib.c >> @@ -2070,7 +2070,7 @@ EXPORT_SYMBOL_GPL(scsi_mode_select); >> /** >> * scsi_mode_sense - issue a mode sense, falling back from 10 to six bytes if necessary. >> * @sdev: SCSI device to be queried >> - * @dbd: set if mode sense will allow block descriptors to be returned >> + * @dbd: set to prevent mode sense from returning block descriptors >> * @modepage: mode page being requested >> * @buffer: request buffer (may not be smaller than eight bytes) >> * @len: length of request buffer. >> @@ -2112,7 +2112,7 @@ scsi_mode_sense(struct scsi_device *sdev, int dbd, int modepage, >> len = 8; >> >> cmd[0] = MODE_SENSE_10; >> - cmd[8] = len; >> + put_unaligned_be16(len, &cmd[7]); >> header_length = 8; >> } else { >> if (len < 4) >> @@ -2166,12 +2166,11 @@ scsi_mode_sense(struct scsi_device *sdev, int dbd, int modepage, >> data->longlba = 0; >> data->block_descriptor_length = 0; >> } else if (use_10_for_ms) { >> - data->length = buffer[0]*256 + buffer[1] + 2; >> + data->length = get_unaligned_be16(&buffer[0]) + 2; >> data->medium_type = buffer[2]; >> data->device_specific = buffer[3]; >> data->longlba = buffer[4] & 0x01; >> - data->block_descriptor_length = buffer[6]*256 >> - + buffer[7]; >> + data->block_descriptor_length = get_unaligned_be16(&buffer[6]); >> } else { >> data->length = buffer[0] + 1; >> data->medium_type = buffer[1]; > > (Nit: > When the subject contains "fix", I think that people automatically look > for a Fixes-tag. However, AFAIK, there isn't a caller that sends in a > length > 1 byte. So a slightly better subject might have been: > "scsi: allow scsi_mode_sense() to read more than 255 bytes") Indeed, that is better. Martin, Can you fix that up ? Or should I resend ? There is also the fact that the argument len is not being checked against 255. So if the device does not have use_10_for_ms set, weird things can happen. I did not add the check because this code has been like this since a long time and no problem is being reported... > > Reviewed-by: Niklas Cassel <niklas.cassel@wdc.com> >
On 8/19/21 12:37 AM, Damien Le Moal wrote: > The allocation length field of the MODE SENSE 10 command is 16-bits, > occupying bytes 7 and 8 of the CDB. With this command, access to mode > pages larger than 255 bytes is thus possible. Make sure that > scsi_mode_sense() code reflects this by initializing the allocation > length field using put_unaligned_be16() instead of directly setting > only byte 8 of the CDB with the buffer length. > > While at it, also fix the folowing: > * use get_unaligned_be16() to retrieve the mode data length and block > descriptor length fields of the mode sense reply header instead of > using an open coded calculation. > * Fix the kdoc dbd argument explanation: the DBD bit stands for > Disable Block Descriptor, which is the opposite of what the dbd > argument description was. > > Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com> > --- > drivers/scsi/scsi_lib.c | 9 ++++----- > 1 file changed, 4 insertions(+), 5 deletions(-) > > diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c > index 7456a26aef51..92db00d81733 100644 > --- a/drivers/scsi/scsi_lib.c > +++ b/drivers/scsi/scsi_lib.c > @@ -2070,7 +2070,7 @@ EXPORT_SYMBOL_GPL(scsi_mode_select); > /** > * scsi_mode_sense - issue a mode sense, falling back from 10 to six bytes if necessary. > * @sdev: SCSI device to be queried > - * @dbd: set if mode sense will allow block descriptors to be returned > + * @dbd: set to prevent mode sense from returning block descriptors > * @modepage: mode page being requested > * @buffer: request buffer (may not be smaller than eight bytes) > * @len: length of request buffer. > @@ -2112,7 +2112,7 @@ scsi_mode_sense(struct scsi_device *sdev, int dbd, int modepage, > len = 8; > > cmd[0] = MODE_SENSE_10; > - cmd[8] = len; > + put_unaligned_be16(len, &cmd[7]); > header_length = 8; > } else { > if (len < 4) > @@ -2166,12 +2166,11 @@ scsi_mode_sense(struct scsi_device *sdev, int dbd, int modepage, > data->longlba = 0; > data->block_descriptor_length = 0; > } else if (use_10_for_ms) { > - data->length = buffer[0]*256 + buffer[1] + 2; > + data->length = get_unaligned_be16(&buffer[0]) + 2; > data->medium_type = buffer[2]; > data->device_specific = buffer[3]; > data->longlba = buffer[4] & 0x01; > - data->block_descriptor_length = buffer[6]*256 > - + buffer[7]; > + data->block_descriptor_length = get_unaligned_be16(&buffer[6]); > } else { > data->length = buffer[0] + 1; > data->medium_type = buffer[1]; If the type of the scsi_mode_sense() 'len' argument is changed from 'int' into 'uint16_t', feel free to add: Reviewed-by: Bart Van Assche <bvanassche@acm.org>
On 2021/08/20 2:04, Bart Van Assche wrote: > On 8/19/21 12:37 AM, Damien Le Moal wrote: >> The allocation length field of the MODE SENSE 10 command is 16-bits, >> occupying bytes 7 and 8 of the CDB. With this command, access to mode >> pages larger than 255 bytes is thus possible. Make sure that >> scsi_mode_sense() code reflects this by initializing the allocation >> length field using put_unaligned_be16() instead of directly setting >> only byte 8 of the CDB with the buffer length. >> >> While at it, also fix the folowing: >> * use get_unaligned_be16() to retrieve the mode data length and block >> descriptor length fields of the mode sense reply header instead of >> using an open coded calculation. >> * Fix the kdoc dbd argument explanation: the DBD bit stands for >> Disable Block Descriptor, which is the opposite of what the dbd >> argument description was. >> >> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com> >> --- >> drivers/scsi/scsi_lib.c | 9 ++++----- >> 1 file changed, 4 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c >> index 7456a26aef51..92db00d81733 100644 >> --- a/drivers/scsi/scsi_lib.c >> +++ b/drivers/scsi/scsi_lib.c >> @@ -2070,7 +2070,7 @@ EXPORT_SYMBOL_GPL(scsi_mode_select); >> /** >> * scsi_mode_sense - issue a mode sense, falling back from 10 to six bytes if necessary. >> * @sdev: SCSI device to be queried >> - * @dbd: set if mode sense will allow block descriptors to be returned >> + * @dbd: set to prevent mode sense from returning block descriptors >> * @modepage: mode page being requested >> * @buffer: request buffer (may not be smaller than eight bytes) >> * @len: length of request buffer. >> @@ -2112,7 +2112,7 @@ scsi_mode_sense(struct scsi_device *sdev, int dbd, int modepage, >> len = 8; >> >> cmd[0] = MODE_SENSE_10; >> - cmd[8] = len; >> + put_unaligned_be16(len, &cmd[7]); >> header_length = 8; >> } else { >> if (len < 4) >> @@ -2166,12 +2166,11 @@ scsi_mode_sense(struct scsi_device *sdev, int dbd, int modepage, >> data->longlba = 0; >> data->block_descriptor_length = 0; >> } else if (use_10_for_ms) { >> - data->length = buffer[0]*256 + buffer[1] + 2; >> + data->length = get_unaligned_be16(&buffer[0]) + 2; >> data->medium_type = buffer[2]; >> data->device_specific = buffer[3]; >> data->longlba = buffer[4] & 0x01; >> - data->block_descriptor_length = buffer[6]*256 >> - + buffer[7]; >> + data->block_descriptor_length = get_unaligned_be16(&buffer[6]); >> } else { >> data->length = buffer[0] + 1; >> data->medium_type = buffer[1]; > > If the type of the scsi_mode_sense() 'len' argument is changed from > 'int' into 'uint16_t', feel free to add: Bart, I would rather keep the argument as an int and add a check for "len > UINT16_MAX" to return an error (-EINVAL) rather than having the interface automatically cast/truncate len values that are too large. Doing so, buggy drivers will get back an error and can be fixed. With the change to uint16_t, errors may end up being hidden. scsi_mode_select() has such check. And looking at that function, it also has problems with the buffer length max possible values as the added length of the header is not accounted for. I fixed that too in a different patch (not sent yet). Thoughts ? > > Reviewed-by: Bart Van Assche <bvanassche@acm.org> > > >
On 8/19/21 7:25 PM, Damien Le Moal wrote: > I would rather keep the argument as an int and add a check for > "len > UINT16_MAX" to return an error (-EINVAL) rather than having the interface > automatically cast/truncate len values that are too large. Doing so, buggy > drivers will get back an error and can be fixed. With the change to uint16_t, > errors may end up being hidden. > > scsi_mode_select() has such check. And looking at that function, it also has > problems with the buffer length max possible values as the added length of the > header is not accounted for. I fixed that too in a different patch (not sent > yet). Thoughts ? Changing the argument type into uint16_t would make it possible for the compiler to warn about integer truncation. The compiler probably would only warn about truncation if the scsi_mode_sense() len argument is a constant. Anyway, I'm also fine with a runtime check for the len argument and keeping its type. Bart.
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c index 7456a26aef51..92db00d81733 100644 --- a/drivers/scsi/scsi_lib.c +++ b/drivers/scsi/scsi_lib.c @@ -2070,7 +2070,7 @@ EXPORT_SYMBOL_GPL(scsi_mode_select); /** * scsi_mode_sense - issue a mode sense, falling back from 10 to six bytes if necessary. * @sdev: SCSI device to be queried - * @dbd: set if mode sense will allow block descriptors to be returned + * @dbd: set to prevent mode sense from returning block descriptors * @modepage: mode page being requested * @buffer: request buffer (may not be smaller than eight bytes) * @len: length of request buffer. @@ -2112,7 +2112,7 @@ scsi_mode_sense(struct scsi_device *sdev, int dbd, int modepage, len = 8; cmd[0] = MODE_SENSE_10; - cmd[8] = len; + put_unaligned_be16(len, &cmd[7]); header_length = 8; } else { if (len < 4) @@ -2166,12 +2166,11 @@ scsi_mode_sense(struct scsi_device *sdev, int dbd, int modepage, data->longlba = 0; data->block_descriptor_length = 0; } else if (use_10_for_ms) { - data->length = buffer[0]*256 + buffer[1] + 2; + data->length = get_unaligned_be16(&buffer[0]) + 2; data->medium_type = buffer[2]; data->device_specific = buffer[3]; data->longlba = buffer[4] & 0x01; - data->block_descriptor_length = buffer[6]*256 - + buffer[7]; + data->block_descriptor_length = get_unaligned_be16(&buffer[6]); } else { data->length = buffer[0] + 1; data->medium_type = buffer[1];
The allocation length field of the MODE SENSE 10 command is 16-bits, occupying bytes 7 and 8 of the CDB. With this command, access to mode pages larger than 255 bytes is thus possible. Make sure that scsi_mode_sense() code reflects this by initializing the allocation length field using put_unaligned_be16() instead of directly setting only byte 8 of the CDB with the buffer length. While at it, also fix the folowing: * use get_unaligned_be16() to retrieve the mode data length and block descriptor length fields of the mode sense reply header instead of using an open coded calculation. * Fix the kdoc dbd argument explanation: the DBD bit stands for Disable Block Descriptor, which is the opposite of what the dbd argument description was. Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com> --- drivers/scsi/scsi_lib.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)