| 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
| ||
|
Date: Thu, 3 Aug 2023 07:08:13 -0600
From: "Gustavo A. R. Silva" <gustavo@...eddedor.com>
To: Jakub Kicinski <kuba@...nel.org>, davem@...emloft.net
Cc: 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 7/27/23 13:07, Jakub Kicinski wrote:
> 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.
It seems I just ran into the same issue you're talking about here. See:
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;
Also, 0-day guys just reported[1] to me a -Wstringop-overflow in a similar piece of code
in the same file. See:
drivers/net/ethernet/broadcom/bnxt/bnxt_dcb.c:133:17: warning: writing 12 bytes into a region of size 1 [-Wstringop-overflow=]
And I think this is a potential solution for that:
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_dcb.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_dcb.c
index 31f85f3e2364..01e0ac246e11 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_dcb.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_dcb.c
@@ -98,7 +98,7 @@ static int bnxt_hwrm_queue_cos2bw_cfg(struct bnxt *bp, struct ieee_ets *ets,
{
struct hwrm_queue_cos2bw_cfg_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_CFG);
@@ -129,8 +129,10 @@ static int bnxt_hwrm_queue_cos2bw_cfg(struct bnxt *bp, struct ieee_ets *ets,
cpu_to_le32((ets->tc_tx_bw[i] * 100) |
BW_VALUE_UNIT_PERCENT1_100);
}
- data = &req->unused_0 + qidx * (sizeof(cos2bw) - 4);
- memcpy(data, &cos2bw.cfg, sizeof(cos2bw) - 4);
+ data = (struct bnxt_cos2bw_cfg *)(&req->unused_0 + qidx *
+ sizeof(cos2bw.cfg) -
+ offsetof(struct bnxt_cos2bw_cfg, cfg));
+ memcpy(&data->cfg, &cos2bw.cfg, sizeof(cos2bw.cfg));
if (qidx == 0) {
req->queue_id0 = cos2bw.queue_id;
req->unused_0 = 0;
If you agree with these changes I can send them as proper patches.
Thanks
--
Gustavo
[1] https://lore.kernel.org/lkml/202308031558.MhRIyeiu-lkp@intel.com/
> ---
> 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;
Powered by blists - more mailing lists