Feedback-ID: i56a14606:Fastmail
User-Agent: Cyrus-JMAP/3.9.0-alpha0-156-g081acc5ed5-fm-20230206.001-g081acc5e
Mime-Version: 1.0
Message-Id: <94b362b4-a30d-4b69-8126-a64f5d025740@app.fastmail.com>
In-Reply-To: <3d94fc3f-fbe5-a56e-3713-4f0788ebc7f9@opensource.wdc.com>
References: <20230213101143.3821483-1-arnd@kernel.org>
 <3d94fc3f-fbe5-a56e-3713-4f0788ebc7f9@opensource.wdc.com>
Date:   Tue, 14 Feb 2023 08:14:48 +0100
From:   "Arnd Bergmann" <arnd@arndb.de>
To:     "Damien Le Moal" <damien.lemoal@opensource.wdc.com>,
        "Arnd Bergmann" <arnd@kernel.org>,
        "Brian King" <brking@us.ibm.com>,
        "James E.J. Bottomley" <jejb@linux.ibm.com>,
        "Martin K. Petersen" <martin.petersen@oracle.com>
Cc:     "Kees Cook" <keescook@chromium.org>,
        "Nathan Chancellor" <nathan@kernel.org>,
        "Nick Desaulniers" <ndesaulniers@google.com>,
        "Tom Rix" <trix@redhat.com>, linux-scsi@vger.kernel.org,
        linux-kernel@vger.kernel.org, llvm@lists.linux.dev
Subject: Re: [PATCH] scsi: ipr: work around fortify-string warning
Content-Type: text/plain
Precedence: bulk

On Tue, Feb 14, 2023, at 04:59, Damien Le Moal wrote:
> On 2/13/23 19:10, Arnd Bergmann wrote:
>> From: Arnd Bergmann <arnd@arndb.de>

>>   **/
>> -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