* [PATCH] scsi: fix scsi_mode_sense()
@ 2021-08-19 7:37 Damien Le Moal
2021-08-19 9:30 ` Niklas Cassel
2021-08-19 17:04 ` Bart Van Assche
0 siblings, 2 replies; 6+ messages in thread
From: Damien Le Moal @ 2021-08-19 7:37 UTC (permalink / raw)
To: linux-scsi, Martin K . Petersen
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];
--
2.31.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] scsi: fix scsi_mode_sense()
2021-08-19 7:37 [PATCH] scsi: fix scsi_mode_sense() Damien Le Moal
@ 2021-08-19 9:30 ` Niklas Cassel
2021-08-19 9:35 ` Damien Le Moal
2021-08-19 17:04 ` Bart Van Assche
1 sibling, 1 reply; 6+ messages in thread
From: Niklas Cassel @ 2021-08-19 9:30 UTC (permalink / raw)
To: Damien Le Moal; +Cc: linux-scsi, Martin K . Petersen
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>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] scsi: fix scsi_mode_sense()
2021-08-19 9:30 ` Niklas Cassel
@ 2021-08-19 9:35 ` Damien Le Moal
0 siblings, 0 replies; 6+ messages in thread
From: Damien Le Moal @ 2021-08-19 9:35 UTC (permalink / raw)
To: Niklas Cassel; +Cc: linux-scsi, Martin K . Petersen
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>
>
--
Damien Le Moal
Western Digital Research
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] scsi: fix scsi_mode_sense()
2021-08-19 7:37 [PATCH] scsi: fix scsi_mode_sense() Damien Le Moal
2021-08-19 9:30 ` Niklas Cassel
@ 2021-08-19 17:04 ` Bart Van Assche
2021-08-20 2:25 ` Damien Le Moal
1 sibling, 1 reply; 6+ messages in thread
From: Bart Van Assche @ 2021-08-19 17:04 UTC (permalink / raw)
To: Damien Le Moal, linux-scsi, Martin K . Petersen
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>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] scsi: fix scsi_mode_sense()
2021-08-19 17:04 ` Bart Van Assche
@ 2021-08-20 2:25 ` Damien Le Moal
2021-08-20 3:57 ` Bart Van Assche
0 siblings, 1 reply; 6+ messages in thread
From: Damien Le Moal @ 2021-08-20 2:25 UTC (permalink / raw)
To: Bart Van Assche, linux-scsi, Martin K . Petersen
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>
>
>
>
--
Damien Le Moal
Western Digital Research
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] scsi: fix scsi_mode_sense()
2021-08-20 2:25 ` Damien Le Moal
@ 2021-08-20 3:57 ` Bart Van Assche
0 siblings, 0 replies; 6+ messages in thread
From: Bart Van Assche @ 2021-08-20 3:57 UTC (permalink / raw)
To: Damien Le Moal, linux-scsi, Martin K . Petersen
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-08-20 3:58 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-19 7:37 [PATCH] scsi: fix scsi_mode_sense() Damien Le Moal
2021-08-19 9:30 ` Niklas Cassel
2021-08-19 9:35 ` Damien Le Moal
2021-08-19 17:04 ` Bart Van Assche
2021-08-20 2:25 ` Damien Le Moal
2021-08-20 3:57 ` Bart Van Assche
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.