Re: [PATCH] scsi: ipr: work around fortify-string warning

[Damien Le Moal <damien.lemoal@opensource.wdc.com>]

On 2/13/23 19:10, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> The ipr_log_vpd_compact() function triggers a fortified memcpy() warning
> about a potential string overflow with all versions of clang:
> 
> In file included from drivers/scsi/ipr.c:43:
> In file included from include/linux/string.h:254:
> include/linux/fortify-string.h:520:4: error: call to '__write_overflow_field' declared with 'warning' attribute: detected write beyond size of field (1st parameter); maybe use struct_group()? [-Werror,-Wattribute-warning]
>                         __write_overflow_field(p_size_field, size);
>                         ^
> include/linux/fortify-string.h:520:4: error: call to '__write_overflow_field' declared with 'warning' attribute: detected write beyond size of field (1st parameter); maybe use struct_group()? [-Werror,-Wattribute-warning]
> 2 errors generated.
> 
> I don't see anything actually wrong with the function, but this is the
> only instance I can reproduce of the fortification going wrong in the
> kernel at the moment, so the easiest solution may be to rewrite the
> function into something that does not trigger the warning.
> 
> Instead of having a combined buffer for vendor/device/serial strings,
> use three separate local variables and just truncate the whitespace
> individually.
> 
> Fixes: 8cf093e275d0 ("[SCSI] ipr: Improved dual adapter errors")
> Cc: Kees Cook <keescook@chromium.org>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> I did not try to bisect which commit introduced this behavior into
> the fortified memcpy(), the Fixes: commit is the one that introduced
> the ipr_log_vpd_compact() function but this predates the fortified
> string helpers.
> ---
>  drivers/scsi/ipr.c | 38 ++++++++++++++++++--------------------
>  1 file changed, 18 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/scsi/ipr.c b/drivers/scsi/ipr.c
> index 198d3f20d682..490fd81e7cfd 100644
> --- a/drivers/scsi/ipr.c
> +++ b/drivers/scsi/ipr.c
> @@ -1516,23 +1516,19 @@ static void ipr_process_ccn(struct ipr_cmnd *ipr_cmd)
>  }
>  
>  /**
> - * strip_and_pad_whitespace - Strip and pad trailing whitespace.
> - * @i:		index into buffer
> - * @buf:		string to modify
> + * strip_whitespace - Strip and pad trailing whitespace.
> + * @i:		size of buffer
> + * @buf:	string to modify
>   *
> - * This function will strip all trailing whitespace, pad the end
> - * of the string with a single space, and NULL terminate the string.
> + * This function will strip all trailing whitespace and
> + * NUL terminate the string.
>   *
> - * Return value:
> - * 	new length of string
>   **/
> -static int strip_and_pad_whitespace(int i, char *buf)
> +static void strip_whitespace(int i, char *buf)
>  {
>  	while (i && buf[i] == ' ')
>  		i--;
> -	buf[i+1] = ' ';
> -	buf[i+2] = '\0';
> -	return i + 2;
> +	buf[i+1] = '\0';

If i is now the size of the buffer, this is a buffer overflow, no ? And
the initial loop should start at "i - 1" I think...

>  }
>  
>  /**
> @@ -1547,19 +1543,21 @@ static int strip_and_pad_whitespace(int i, char *buf)
>  static void ipr_log_vpd_compact(char *prefix, struct ipr_hostrcb *hostrcb,
>  				struct ipr_vpd *vpd)
>  {
> -	char buffer[IPR_VENDOR_ID_LEN + IPR_PROD_ID_LEN + IPR_SERIAL_NUM_LEN + 3];
> -	int i = 0;
> +	char vendor_id[IPR_VENDOR_ID_LEN + 1];

...but the size is in fact "i + 1"... So in strip_whitespace(), i is the
index of the last possible character in the string, and given that the
string may be much shorter, that function may not actually strip
whitespaces after the string...

> +	char product_id[IPR_PROD_ID_LEN + 1];
> +	char sn[IPR_SERIAL_NUM_LEN + 1];
>  
> -	memcpy(buffer, vpd->vpids.vendor_id, IPR_VENDOR_ID_LEN);
> -	i = strip_and_pad_whitespace(IPR_VENDOR_ID_LEN - 1, buffer);
> +	memcpy(vendor_id, vpd->vpids.vendor_id, IPR_VENDOR_ID_LEN);
> +	strip_whitespace(IPR_VENDOR_ID_LEN, vendor_id);

So this call should really be:

	strip_whitespace(strlen(vendor_id) - 1, vendor_id);

Which means that this helper can be turned into:

static void strip_whitespace(char *buf)
{
	int i = strlen(buf) - 1;

	while (i > 0 && buf[i] == ' ')
		i--;
	buf[i+1] = '\0';
}

Unless I am missing something :)
>  
> -	memcpy(&buffer[i], vpd->vpids.product_id, IPR_PROD_ID_LEN);
> -	i = strip_and_pad_whitespace(i + IPR_PROD_ID_LEN - 1, buffer);
> +	memcpy(product_id, vpd->vpids.product_id, IPR_PROD_ID_LEN);
> +	strip_whitespace(IPR_PROD_ID_LEN, product_id);
>  
> -	memcpy(&buffer[i], vpd->sn, IPR_SERIAL_NUM_LEN);
> -	buffer[IPR_SERIAL_NUM_LEN + i] = '\0';
> +	memcpy(sn, vpd->sn, IPR_SERIAL_NUM_LEN);
> +	strip_whitespace(IPR_SERIAL_NUM_LEN, sn);
>  
> -	ipr_hcam_err(hostrcb, "%s VPID/SN: %s\n", prefix, buffer);
> +	ipr_hcam_err(hostrcb, "%s VPID/SN: %s %s %s\n", prefix,
> +		     vendor_id, product_id, sn);
>  }
>  
>  /**

-- 
Damien Le Moal
Western Digital Research