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]

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

On Thu, Aug 3, 2023 at 7:21 AM Jakub Kicinski <kuba@...nel.org> wrote:
>
> 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.

The way I plan to fix this is to change the auto-generated struct
hwrm_queue_cos2bw_qcfg_output to have an array of substruct.  I think
that will look the cleanest.  I'll post it later today or tomorrow.

Download attachment "smime.p7s" of type "application/pkcs7-signature" (4209 bytes)