Re: [PATCH 1/3][next] scsi: smartpqi: Replace one-element array with flexible-array member

[Kees Cook]

On Wed, Sep 21, 2022 at 11:28:35PM -0500, Gustavo A. R. Silva wrote:
> One-element arrays are deprecated, and we are replacing them with flexible
> array members instead. So, replace one-element array with flexible-array
> member in struct MR_DRV_RAID_MAP and refactor the the rest of the code
> accordingly.
> 
> It seems that the addition of sizeof(struct report_log_lun) in all the
> places that are modified by this patch is due to the fact that
> the one-element array struct report_log_lun lun_entries[1]; always
> contributes to the size of the containing structure struct
> report_log_lun_list.
> 
> Notice that at line 1267 while allocating memory for an instance of
> struct report_log_lun_list, some _extra_ space seems to be allocated
> for one element of type struct report_log_lun, which is the type of
> the elements in array lun_entries:
> 
>  1267         internal_logdev_list = kmalloc(logdev_data_length +
>  1268                 sizeof(struct report_log_lun), GFP_KERNEL);
> 
> However, at line 1275 just logdev_data_length bytes are copied into
> internal_logdev_list (remember that we allocated space for logdev_data_length +
> sizeof(struct report_log_lun) bytes at line 1267), and then exactly
> sizeof(struct report_log_lun) bytes are being zeroing out at line 1276.
> 
>  1275         memcpy(internal_logdev_list, logdev_data, logdev_data_length);
>  1276         memset((u8 *)internal_logdev_list + logdev_data_length, 0,
>  1277                 sizeof(struct report_log_lun));
> 
> All the above makes think that it's just fine if we transform array
> lun_entries into a flexible-array member and just don't allocate
> that extra sizeof(struct report_log_lun) bytes of space. With this
> we can remove that memset() call and we also need to modify the code
> that updates the total length (internal_logdev_list->header.list_length)
> of array lun_entries at line 1278:
> 
>  1278         put_unaligned_be32(logdev_list_length +
>  1279                 sizeof(struct report_log_lun),
>  1280                 &internal_logdev_list->header.list_length);
> 
> This helps with the ongoing efforts to tighten the FORTIFY_SOURCE routines
> on memcpy().
> 
> Link: https://github.com/KSPP/linux/issues/79
> Link: https://github.com/KSPP/linux/issues/204
> Signed-off-by: Gustavo A. R. Silva <gustavoars@xxxxxxxxxx>
> ---
> And of course, it'd be great if maintainers can confirm what I described
> in the changelog text. :)
> 
>  drivers/scsi/smartpqi/smartpqi.h      |  2 +-
>  drivers/scsi/smartpqi/smartpqi_init.c | 10 +++-------
>  2 files changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/scsi/smartpqi/smartpqi.h b/drivers/scsi/smartpqi/smartpqi.h
> index e550b12e525a..d1756c9d1112 100644
> --- a/drivers/scsi/smartpqi/smartpqi.h
> +++ b/drivers/scsi/smartpqi/smartpqi.h
> @@ -954,7 +954,7 @@ struct report_log_lun {
>  
>  struct report_log_lun_list {
>  	struct report_lun_header header;
> -	struct report_log_lun lun_entries[1];
> +	struct report_log_lun lun_entries[];
>  };
>  
>  struct report_phys_lun_8byte_wwid {
> diff --git a/drivers/scsi/smartpqi/smartpqi_init.c b/drivers/scsi/smartpqi/smartpqi_init.c
> index b971fbe3b3a1..544cd18a90d7 100644
> --- a/drivers/scsi/smartpqi/smartpqi_init.c
> +++ b/drivers/scsi/smartpqi/smartpqi_init.c
> @@ -1264,8 +1264,7 @@ static int pqi_get_device_lists(struct pqi_ctrl_info *ctrl_info,
>  	logdev_data_length = sizeof(struct report_lun_header) +
>  		logdev_list_length;
>  
> -	internal_logdev_list = kmalloc(logdev_data_length +
> -		sizeof(struct report_log_lun), GFP_KERNEL);
> +	internal_logdev_list = kmalloc(logdev_data_length, GFP_KERNEL);
>  	if (!internal_logdev_list) {
>  		kfree(*logdev_list);
>  		*logdev_list = NULL;
> @@ -1273,11 +1272,8 @@ static int pqi_get_device_lists(struct pqi_ctrl_info *ctrl_info,
>  	}
>  
>  	memcpy(internal_logdev_list, logdev_data, logdev_data_length);
> -	memset((u8 *)internal_logdev_list + logdev_data_length, 0,
> -		sizeof(struct report_log_lun));
> -	put_unaligned_be32(logdev_list_length +
> -		sizeof(struct report_log_lun),
> -		&internal_logdev_list->header.list_length);
> +	put_unaligned_be32(logdev_list_length,
> +			   &internal_logdev_list->header.list_length);

This is a bit of a twisty maze to read through, but at the end, I agree
with your assessment: it was needlessly added the extra member to the
allocation. I don't see it used anywhere in later code -- it's always
bounded by logdev_list_length.

Reviewed-by: Kees Cook <keescook@xxxxxxxxxxxx>

>  
>  	kfree(*logdev_list);
>  	*logdev_list = internal_logdev_list;
> -- 
> 2.34.1
> 

-- 
Kees Cook