From: Arnd Bergmann <[email protected]>
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 <[email protected]>
Signed-off-by: Arnd Bergmann <[email protected]>
---
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';
}
/**
@@ -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];
+ 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);
- 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);
}
/**
--
2.39.1
On 2/13/23 19:10, Arnd Bergmann wrote:
> From: Arnd Bergmann <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Arnd Bergmann <[email protected]>
> ---
> 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
On Tue, Feb 14, 2023, at 04:59, Damien Le Moal wrote:
> On 2/13/23 19:10, Arnd Bergmann wrote:
>> From: Arnd Bergmann <[email protected]>
>> **/
>> -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...
Right, I definitely have the wrong length here.
>> }
>>
>> /**
>> @@ -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...
I think aside from the off-by-one bug I introduced, this is not
a (new) problem as the old code already assumed that the input
is padded with spaces rather than nul-terminated.
>> + 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 :)
The strlen() here requires the input to be a properly terminated string,
so this would at least require adding a \0.
Also, if the input is a short nul-terminated string instead of
a space padded array, we probably don't need to strip the trailing
whitespace, or at least the original version didn't either.
Let me try to respin my patch with just the off-by-one error
fixed but otherwise keeping the output of ipr_log_vpd_compact()
unchanged.
Arnd
On 2/14/23 16:14, Arnd Bergmann wrote:
> On Tue, Feb 14, 2023, at 04:59, Damien Le Moal wrote:
>> On 2/13/23 19:10, Arnd Bergmann wrote:
>>> From: Arnd Bergmann <[email protected]>
>
>>> **/
>>> -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...
>
> Right, I definitely have the wrong length here.
>
>>> }
>>>
>>> /**
>>> @@ -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...
>
> I think aside from the off-by-one bug I introduced, this is not
> a (new) problem as the old code already assumed that the input
> is padded with spaces rather than nul-terminated.
Yeah. The HW probably always give the same amount of chars with short
strings completed by spaces...
>
>>> + 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 :)
>
> The strlen() here requires the input to be a properly terminated string,
> so this would at least require adding a \0.
>
> Also, if the input is a short nul-terminated string instead of
> a space padded array, we probably don't need to strip the trailing
> whitespace, or at least the original version didn't either.
>
> Let me try to respin my patch with just the off-by-one error
> fixed but otherwise keeping the output of ipr_log_vpd_compact()
> unchanged.
>
> Arnd
--
Damien Le Moal
Western Digital Research