netdev - Re: [PATCH net-next] bnx2x: Set ivi->vlan field as an integer

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: <deafd990-8186-4fc3-8ec5-46a1e9ad7501@amd.com>
Date: Thu, 15 Aug 2024 10:55:04 -0700
From: Brett Creeley <bcreeley@....com>
To: Simon Horman <horms@...nel.org>, Sudarsana Kalluru
 <skalluru@...vell.com>, Manish Chopra <manishc@...vell.com>
Cc: "David S. Miller" <davem@...emloft.net>,
 Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
 Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org
Subject: Re: [PATCH net-next] bnx2x: Set ivi->vlan field as an integer



On 8/15/2024 8:27 AM, Simon Horman wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
> 
> 
> In bnx2x_get_vf_config():
> * The vlan field of ivi is a 32-bit integer, it is used to store a vlan ID.
> * The vlan field of bulletin is a 16-bit integer, it is also used to store
>    a vlan ID.
> 
> In the current code, ivi->vlan is set using memset. But in the case of
> setting it to the value of bulletin->vlan, this involves reading
> 32 bits from a 16bit source. This is likely safe, as the following
> 6 bytes are padding in the same structure, but none the less, it seems
> undesirable.
> 
> However, it is entirely unclear to me how this scheme works on
> big-endian systems.
> 
> Resolve this by simply assigning integer values to ivi->vlan.
> 
> Flagged by W=1 builds.
> f.e. gcc-14 reports:
> 
> In function 'fortify_memcpy_chk',
>      inlined from 'bnx2x_get_vf_config' at .../bnx2x_sriov.c:2655:4:
> .../fortify-string.h:580: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]
>    580 |                         __read_overflow2_field(q_size_field, size);
>        |                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> Compile tested only.
> 
> Signed-off-by: Simon Horman <horms@...nel.org>
> ---
>   drivers/net/ethernet/broadcom/bnx2x/bnx2x_sriov.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_sriov.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_sriov.c
> index 77d4cb4ad782..12198fc3ab22 100644
> --- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_sriov.c
> +++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_sriov.c
> @@ -2652,10 +2652,10 @@ int bnx2x_get_vf_config(struct net_device *dev, int vfidx,
>                  /* vlan */
>                  if (bulletin->valid_bitmap & (1 << VLAN_VALID))
>                          /* vlan configured by ndo so its in bulletin board */
> -                       memcpy(&ivi->vlan, &bulletin->vlan, VLAN_HLEN);
> +                       ivi->vlan = bulletin->vlan;
>                  else
>                          /* function has not been loaded yet. Show vlans as 0s */
> -                       memset(&ivi->vlan, 0, VLAN_HLEN);
> +                       ivi->vlan = 0;

Makes sense to me.

Reviewed-by: Brett Creeley <brett.creeley@....com>

> 
>                  mutex_unlock(&bp->vfdb->bulletin_mutex);
>          }
> 
>