| Message ID | 20240815-bnx2x-int-vlan-v1-1-5940b76e37ad@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | a99ef548bba01435f19137cf1670861be1c1ee4b |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] bnx2x: Set ivi->vlan field as an integer | expand |
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@kernel.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@amd.com> > > mutex_unlock(&bp->vfdb->bulletin_mutex); > } > >
Hello: This patch was applied to netdev/net-next.git (main) by Jakub Kicinski <kuba@kernel.org>: On Thu, 15 Aug 2024 16:27:46 +0100 you wrote: > 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. > > [...] Here is the summary with links: - [net-next] bnx2x: Set ivi->vlan field as an integer https://git.kernel.org/netdev/net-next/c/a99ef548bba0 You are awesome, thank you!
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; mutex_unlock(&bp->vfdb->bulletin_mutex); }
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@kernel.org> --- drivers/net/ethernet/broadcom/bnx2x/bnx2x_sriov.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)