On 2/13/23 19:10, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@xxxxxxxx>
>
> 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@xxxxxxxxxxxx>
> Signed-off-by: Arnd Bergmann <arnd@xxxxxxxx>
> ---
> 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
