netdev - [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 for Android: free password hash cracker in your pocket

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

Date: Thu, 27 Jul 2023 12:07:25 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: davem@...emloft.net
Cc: netdev@...r.kernel.org,
	edumazet@...gle.com,
	pabeni@...hat.com,
	michael.chan@...adcom.com,
	Jakub Kicinski <kuba@...nel.org>
Subject: [PATCH net-next 1/2] eth: bnxt: fix one of the W=1 warnings about fortified memcpy()

Fix a W=1 warning with gcc 13.1:

In function ‘fortify_memcpy_chk’,
    inlined from ‘bnxt_hwrm_queue_cos2bw_cfg’ at drivers/net/ethernet/broadcom/bnxt/bnxt_dcb.c:133: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);
      |                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

The field group is already defined and starts at queue_id:

struct bnxt_cos2bw_cfg {
	u8			pad[3];
	struct_group_attr(cfg, __packed,
		u8		queue_id;
		__le32		min_bw;

Signed-off-by: Jakub Kicinski <kuba@...nel.org>
---
CC: michael.chan@...adcom.com

Michael, the other warning looks more.. interesting.
The code does:

	data = &resp->queue_id0 + offsetof(struct bnxt_cos2bw_cfg, queue_id);

which translates to: data = &resp->queue_id0 + 3, but the HWRM struct
says:

struct hwrm_queue_cos2bw_qcfg_output {
	__le16	error_code;
	__le16	req_type;
	__le16	seq_id;
	__le16	resp_len;
	u8	queue_id0;
	u8	unused_0;
	__le16	unused_1;
	__le32	queue_id0_min_bw;

So queue_id0 is in the wrong place?
Why not just move it in the spec?
Simplest fix for the warning would be to assign the 6 fields by value.
But to get the value of queue_id we'd need to read unused_1 AFACT? :o
Could you please fix this somehow? Doing W=1 builds on bnxt is painful.
---
 drivers/net/ethernet/broadcom/bnxt/bnxt_dcb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_dcb.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_dcb.c
index caab3d626a2a..31f85f3e2364 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_dcb.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_dcb.c
@@ -130,7 +130,7 @@ static int bnxt_hwrm_queue_cos2bw_cfg(struct bnxt *bp, struct ieee_ets *ets,
 					    BW_VALUE_UNIT_PERCENT1_100);
 		}
 		data = &req->unused_0 + qidx * (sizeof(cos2bw) - 4);
-		memcpy(data, &cos2bw.queue_id, sizeof(cos2bw) - 4);
+		memcpy(data, &cos2bw.cfg, sizeof(cos2bw) - 4);
 		if (qidx == 0) {
 			req->queue_id0 = cos2bw.queue_id;
 			req->unused_0 = 0;
-- 
2.41.0