netdev - Re: [PATCH net-next 1/2] eth: bnxt: fix one of the W=1 warnings about fortified memcpy()

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20230803072123.1fbd56db@kernel.org>
Date: Thu, 3 Aug 2023 07:21:23 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: "Gustavo A. R. Silva" <gustavo@...eddedor.com>
Cc: davem@...emloft.net, netdev@...r.kernel.org, edumazet@...gle.com,
 pabeni@...hat.com, michael.chan@...adcom.com
Subject: Re: [PATCH net-next 1/2] eth: bnxt: fix one of the W=1 warnings
 about fortified memcpy()

On Thu, 3 Aug 2023 07:08:13 -0600 Gustavo A. R. Silva wrote:
> In function 'fortify_memcpy_chk',
>      inlined from 'bnxt_hwrm_queue_cos2bw_qcfg' at drivers/net/ethernet/broadcom/bnxt/bnxt_dcb.c:165:3:
> include/linux/fortify-string.h:592:25: warning: call to '__read_overflow2_field' declared with attribute warning: detected read beyond size of field (2nd 
> parameter); maybe use struct_group()? [-Wattribute-warning]
>    592 |                         __read_overflow2_field(q_size_field, size);
>        |                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> Here is a potential fix for that:
> 
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_dcb.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_dcb.c
> index 31f85f3e2364..e2390d73b3f0 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_dcb.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_dcb.c
> @@ -144,7 +144,7 @@ static int bnxt_hwrm_queue_cos2bw_qcfg(struct bnxt *bp, struct ieee_ets *ets)
>          struct hwrm_queue_cos2bw_qcfg_output *resp;
>          struct hwrm_queue_cos2bw_qcfg_input *req;
>          struct bnxt_cos2bw_cfg cos2bw;
> -       void *data;
> +       struct bnxt_cos2bw_cfg *data;
>          int rc, i;
> 
>          rc = hwrm_req_init(bp, req, HWRM_QUEUE_COS2BW_QCFG);
> @@ -158,11 +158,11 @@ static int bnxt_hwrm_queue_cos2bw_qcfg(struct bnxt *bp, struct ieee_ets *ets)
>                  return rc;
>          }
> 
> -       data = &resp->queue_id0 + offsetof(struct bnxt_cos2bw_cfg, queue_id);
> +       data = (struct bnxt_cos2bw_cfg *)&resp->queue_id0;
>          for (i = 0; i < bp->max_tc; i++, data += sizeof(cos2bw.cfg)) {
>                  int tc;
> 
> -               memcpy(&cos2bw.cfg, data, sizeof(cos2bw.cfg));
> +               memcpy(&cos2bw.cfg, &data->cfg, sizeof(cos2bw.cfg));
>                  if (i == 0)
>                          cos2bw.queue_id = resp->queue_id0;

Neat trick, but seems like casting to the destination type should
really be the last resort. There's only a handful of members in this
struct, IMHO assigning member by member is cleaner.
But I'll defer to Michael.