Re: [PATCH v2 3/9] scsi: sd: Pass buffer length as argument to sd_read_capacity() et al.

[Damien Le Moal <dlemoal@kernel.org>]

On 8/15/25 03:29, Ewan D. Milne wrote:
> That will make it easier to spot an inconsistent buffer size value.

How ? the buffer size is actually not checked. You only memset the buffer.

> Also, memset() the entire buffer rather than the 8 or 32 bytes expected
> back from READ CAPACITY(10) or READ CAPACITY(16).

Why ? There is no explanation why that is needed. The command will not return
more than the transfer length specified. So memsetting bytes that will never be
used seems useless.

> 
> Signed-off-by: Ewan D. Milne <emilne@redhat.com>
> ---
>  drivers/scsi/sd.c | 28 ++++++++++++++++++----------
>  1 file changed, 18 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index e3b802b26f0e..ae8eac4b1cb2 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -2629,7 +2629,8 @@ static void read_capacity_error(struct scsi_disk *sdkp, struct scsi_device *sdp,
>  #define READ_CAPACITY_RETRIES_ON_RESET	10
>  
>  static int read_capacity_16(struct scsi_disk *sdkp, struct scsi_device *sdp,
> -		struct queue_limits *lim, unsigned char *buffer)
> +			    struct queue_limits *lim, unsigned char *buffer,
> +			    unsigned int buflen)
>  {
>  	unsigned char cmd[16];
>  	struct scsi_sense_hdr sshdr;
> @@ -2651,7 +2652,7 @@ static int read_capacity_16(struct scsi_disk *sdkp, struct scsi_device *sdp,
>  		cmd[0] = SERVICE_ACTION_IN_16;
>  		cmd[1] = SAI_READ_CAPACITY_16;
>  		cmd[13] = RC16_LEN;
> -		memset(buffer, 0, RC16_LEN);
> +		memset(buffer, 0, buflen);

I would leave this as-is.

>  >  		the_result = scsi_execute_cmd(sdp, cmd, REQ_OP_DRV_IN,
>  					      buffer, RC16_LEN, SD_TIMEOUT,
> @@ -2719,8 +2720,13 @@ static int read_capacity_16(struct scsi_disk *sdkp, struct scsi_device *sdp,
>  	return sector_size;
>  }
>  
> +#define RC10_LEN 8
> +#if RC10_LEN > SD_BUF_SIZE
> +#error RC10_LEN must not be more than SD_BUF_SIZE
> +#endif
> +
>  static int read_capacity_10(struct scsi_disk *sdkp, struct scsi_device *sdp,
> -						unsigned char *buffer)
> +			    unsigned char *buffer, unsigned int buflen)
>  {
>  	static const u8 cmd[10] = { READ_CAPACITY };
>  	struct scsi_sense_hdr sshdr;
> @@ -2765,7 +2771,7 @@ static int read_capacity_10(struct scsi_disk *sdkp, struct scsi_device *sdp,
>  	sector_t lba;
>  	unsigned sector_size;
>  
> -	memset(buffer, 0, 8);
> +	memset(buffer, 0, buflen);

Same here, but maybe define RC10_LEN instead of having the magic "8" value
hardcoded ?

-- 
Damien Le Moal
Western Digital Research