Re: [PATCH] bfa: avoid buffer overrun for 12-byte model name

[Jim Meyering]

Krishna Gudipati wrote:
>> -----Original Message-----
>> From: Jim Meyering [mailto:jim@xxxxxxxxxxxx]
>> Sent: Monday, August 20, 2012 9:55 AM
>> To: linux-kernel@xxxxxxxxxxxxxxx
>> Cc: Jim Meyering; Jing Huang; Krishna Gudipati; James E.J. Bottomley; linux-
>> scsi@xxxxxxxxxxxxxxx
>> Subject: [PATCH] bfa: avoid buffer overrun for 12-byte model name
>>
>> From: Jim Meyering <meyering@xxxxxxxxxx>
>>
>> we use strncpy to copy a model name of length up to 15 (16, if you count the
>> NUL), into a buffer of size 12 (BFA_FCS_PORT_SYMBNAME_MODEL_SZ).
>> However, strncpy does not always NUL-terminate, so whenever the original
>> model string has strlen >= 12, the following strncat reads beyond end of the -
>> >sym_name buffer as it attempts to find end of string.
>>
>> bfa_fcs_fabric_psymb_init(struct bfa_fcs_fabric_s *fabric) {
>> 	bfa_ioc_get_adapter_model(&fabric->fcs->bfa->ioc, model);
>> 	...
>> 	strncpy((char *)&port_cfg->sym_name, model,
>> 		BFA_FCS_PORT_SYMBNAME_MODEL_SZ);
>> 	strncat((char *)&port_cfg->sym_name,
>> BFA_FCS_PORT_SYMBNAME_SEPARATOR,
>> 		sizeof(BFA_FCS_PORT_SYMBNAME_SEPARATOR));
>> 	...
>>
>> bfa_ioc_get_adapter_model(struct bfa_ioc_s *ioc, char *model) {
>> 	struct bfi_ioc_attr_s	*ioc_attr;
>>
>> 	WARN_ON(!model);
>> 	memset((void *)model, 0, BFA_ADAPTER_MODEL_NAME_LEN);
>>
>> BFA_ADAPTER_MODEL_NAME_LEN = 16
>>
>> Signed-off-by: Jim Meyering <meyering@xxxxxxxxxx>
>> ---
>>  drivers/scsi/bfa/bfa_fcs.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/scsi/bfa/bfa_fcs.c b/drivers/scsi/bfa/bfa_fcs.c index
>> eaac57e..3329493 100644
>> --- a/drivers/scsi/bfa/bfa_fcs.c
>> +++ b/drivers/scsi/bfa/bfa_fcs.c
>> @@ -713,6 +713,7 @@ bfa_fcs_fabric_psymb_init(struct bfa_fcs_fabric_s
>> *fabric)
>>  	/* Model name/number */
>>  	strncpy((char *)&port_cfg->sym_name, model,
>>  		BFA_FCS_PORT_SYMBNAME_MODEL_SZ);
>> +	port_cfg->sym_name[BFA_FCS_PORT_SYMBNAME_MODEL_SZ - 1]
>> = 0;
>>  	strncat((char *)&port_cfg->sym_name,
>> BFA_FCS_PORT_SYMBNAME_SEPARATOR,
>>  		sizeof(BFA_FCS_PORT_SYMBNAME_SEPARATOR));
>
> Nacked-by: Krishna Gudipati <kgudipat@xxxxxxxxxxx>
>
> Hi Jim,
>
> This model number is of length 12 bytes and the logic added here will
> reset the model last byte.
> In addition strncat does not need the src to be null terminated, the
> change does not compile even.
> NACK to this change.

Hi Krishna,

Thanks for the quick feedback and sorry the patch wasn't quite right.
However, the log is accurate: there is at least a theoretical problem
when the string in "model" (a buffer of size 16 bytes) has strlen >= 12.
While strncat does not require that its second argument be NUL-terminated,
the first one (the destination) must be.  Otherwise, it has no way to
determine the end of the string to which it must append the source bytes.

Here is a v2 patch to which I've added the requisite (char*) cast.
However, this whole function is rather unreadable due to the
repetition (12 times!) of "(char *)&port_cfg->sym_name".
In case someone prefers to factor out that repetition,
I've appended a larger, v3 patch to do that.