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

[Krishna Gudipati]

> -----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.

Thanks,
Krishna
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/