* [PATCH v3 0/2] Fix out-of-bounds warnings
@ 2021-04-14 23:40 Gustavo A. R. Silva
2021-04-14 23:43 ` [PATCH v3 1/2] wl3501_cs: Fix out-of-bounds warnings in wl3501_send_pkt Gustavo A. R. Silva
2021-04-14 23:45 ` [PATCH v3 2/2] wl3501_cs: Fix out-of-bounds warnings in wl3501_mgmt_join Gustavo A. R. Silva
0 siblings, 2 replies; 7+ messages in thread
From: Gustavo A. R. Silva @ 2021-04-14 23:40 UTC (permalink / raw)
To: linux-kernel
Cc: Kalle Valo, David S. Miller, Jakub Kicinski, linux-wireless,
netdev, linux-hardening, Gustavo A. R. Silva, Kees Cook
Fix multiple out-of-bounds warnings by making the code a bit more
structured.
This helps with the ongoing efforts to globally enable -Warray-bounds
and get us closer to being able to tighten the FORTIFY_SOURCE routines
on memcpy().
Link: https://github.com/KSPP/linux/issues/109
Changes in v3:
- Add new struct wl3501_req.
- Update changelog text in patch 2/2.
- Add Kees' RB tag to patch 1/2.
- Fix one more instance of this same issue in both patches.
Changes in v2:
- Update changelog text in patch 1/2.
- Replace a couple of magic numbers with new variable sig_addr_len.
Gustavo A. R. Silva (2):
wl3501_cs: Fix out-of-bounds warnings in wl3501_send_pkt
wl3501_cs: Fix out-of-bounds warnings in wl3501_mgmt_join
drivers/net/wireless/wl3501.h | 47 ++++++++++++++-------------
drivers/net/wireless/wl3501_cs.c | 54 +++++++++++++++++---------------
2 files changed, 52 insertions(+), 49 deletions(-)
--
2.27.0
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v3 1/2] wl3501_cs: Fix out-of-bounds warnings in wl3501_send_pkt
2021-04-14 23:40 [PATCH v3 0/2] Fix out-of-bounds warnings Gustavo A. R. Silva
@ 2021-04-14 23:43 ` Gustavo A. R. Silva
2021-04-22 14:39 ` Kalle Valo
[not found] ` <20210422143910.C8B5CC4338A@smtp.codeaurora.org>
2021-04-14 23:45 ` [PATCH v3 2/2] wl3501_cs: Fix out-of-bounds warnings in wl3501_mgmt_join Gustavo A. R. Silva
1 sibling, 2 replies; 7+ messages in thread
From: Gustavo A. R. Silva @ 2021-04-14 23:43 UTC (permalink / raw)
To: linux-kernel
Cc: Kalle Valo, David S. Miller, Jakub Kicinski, linux-wireless,
netdev, linux-hardening, Gustavo A. R. Silva, Kees Cook
Fix the following out-of-bounds warnings by enclosing structure members
daddr and saddr into new struct addr, in structures wl3501_md_req and
wl3501_md_ind:
arch/x86/include/asm/string_32.h:182:25: warning: '__builtin_memcpy' offset [18, 23] from the object at 'sig' is out of the bounds of referenced subobject 'daddr' with type 'u8[6]' {aka 'unsigned char[6]'} at offset 11 [-Warray-bounds]
arch/x86/include/asm/string_32.h:182:25: warning: '__builtin_memcpy' offset [18, 23] from the object at 'sig' is out of the bounds of referenced subobject 'daddr' with type 'u8[6]' {aka 'unsigned char[6]'} at offset 11 [-Warray-bounds]
Refactor the code, accordingly:
$ pahole -C wl3501_md_req drivers/net/wireless/wl3501_cs.o
struct wl3501_md_req {
u16 next_blk; /* 0 2 */
u8 sig_id; /* 2 1 */
u8 routing; /* 3 1 */
u16 data; /* 4 2 */
u16 size; /* 6 2 */
u8 pri; /* 8 1 */
u8 service_class; /* 9 1 */
struct {
u8 daddr[6]; /* 10 6 */
u8 saddr[6]; /* 16 6 */
} addr; /* 10 12 */
/* size: 22, cachelines: 1, members: 8 */
/* last cacheline: 22 bytes */
};
$ pahole -C wl3501_md_ind drivers/net/wireless/wl3501_cs.o
struct wl3501_md_ind {
u16 next_blk; /* 0 2 */
u8 sig_id; /* 2 1 */
u8 routing; /* 3 1 */
u16 data; /* 4 2 */
u16 size; /* 6 2 */
u8 reception; /* 8 1 */
u8 pri; /* 9 1 */
u8 service_class; /* 10 1 */
struct {
u8 daddr[6]; /* 11 6 */
u8 saddr[6]; /* 17 6 */
} addr; /* 11 12 */
/* size: 24, cachelines: 1, members: 9 */
/* padding: 1 */
/* last cacheline: 24 bytes */
};
The problem is that the original code is trying to copy data into a
couple of arrays adjacent to each other in a single call to memcpy().
Now that a new struct _addr_ enclosing those two adjacent arrays
is introduced, memcpy() doesn't overrun the length of &sig.daddr[0]
and &sig.daddr, because the address of the new struct object _addr_
is used, instead.
This helps with the ongoing efforts to globally enable -Warray-bounds
and get us closer to being able to tighten the FORTIFY_SOURCE routines
on memcpy().
Link: https://github.com/KSPP/linux/issues/109
Reported-by: kernel test robot <lkp@intel.com>
Reviewed-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
---
Changes in v3:
- Enclose adjacent members in struct wl3501_md_ind into new struct req.
- Fix one more instance of this same issue in function
wl3501_md_ind_interrupt().
- Update changelog text.
- Add Kees' RB tag.
Changes in v2:
- Update changelog text.
- Replace a couple of magic numbers with new variable sig_addr_len.
drivers/net/wireless/wl3501.h | 12 ++++++++----
drivers/net/wireless/wl3501_cs.c | 10 ++++++----
2 files changed, 14 insertions(+), 8 deletions(-)
diff --git a/drivers/net/wireless/wl3501.h b/drivers/net/wireless/wl3501.h
index e98e04ee9a2c..aa8222cbea68 100644
--- a/drivers/net/wireless/wl3501.h
+++ b/drivers/net/wireless/wl3501.h
@@ -471,8 +471,10 @@ struct wl3501_md_req {
u16 size;
u8 pri;
u8 service_class;
- u8 daddr[ETH_ALEN];
- u8 saddr[ETH_ALEN];
+ struct {
+ u8 daddr[ETH_ALEN];
+ u8 saddr[ETH_ALEN];
+ } addr;
};
struct wl3501_md_ind {
@@ -484,8 +486,10 @@ struct wl3501_md_ind {
u8 reception;
u8 pri;
u8 service_class;
- u8 daddr[ETH_ALEN];
- u8 saddr[ETH_ALEN];
+ struct {
+ u8 daddr[ETH_ALEN];
+ u8 saddr[ETH_ALEN];
+ } addr;
};
struct wl3501_md_confirm {
diff --git a/drivers/net/wireless/wl3501_cs.c b/drivers/net/wireless/wl3501_cs.c
index 8ca5789c7b37..70307308635f 100644
--- a/drivers/net/wireless/wl3501_cs.c
+++ b/drivers/net/wireless/wl3501_cs.c
@@ -469,6 +469,7 @@ static int wl3501_send_pkt(struct wl3501_card *this, u8 *data, u16 len)
struct wl3501_md_req sig = {
.sig_id = WL3501_SIG_MD_REQ,
};
+ size_t sig_addr_len = sizeof(sig.addr);
u8 *pdata = (char *)data;
int rc = -EIO;
@@ -484,9 +485,9 @@ static int wl3501_send_pkt(struct wl3501_card *this, u8 *data, u16 len)
goto out;
}
rc = 0;
- memcpy(&sig.daddr[0], pdata, 12);
- pktlen = len - 12;
- pdata += 12;
+ memcpy(&sig.addr, pdata, sig_addr_len);
+ pktlen = len - sig_addr_len;
+ pdata += sig_addr_len;
sig.data = bf;
if (((*pdata) * 256 + (*(pdata + 1))) > 1500) {
u8 addr4[ETH_ALEN] = {
@@ -980,7 +981,8 @@ static inline void wl3501_md_ind_interrupt(struct net_device *dev,
} else {
skb->dev = dev;
skb_reserve(skb, 2); /* IP headers on 16 bytes boundaries */
- skb_copy_to_linear_data(skb, (unsigned char *)&sig.daddr, 12);
+ skb_copy_to_linear_data(skb, (unsigned char *)&sig.addr,
+ sizeof(sig.addr));
wl3501_receive(this, skb->data, pkt_len);
skb_put(skb, pkt_len);
skb->protocol = eth_type_trans(skb, dev);
--
2.27.0
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH v3 2/2] wl3501_cs: Fix out-of-bounds warnings in wl3501_mgmt_join
2021-04-14 23:40 [PATCH v3 0/2] Fix out-of-bounds warnings Gustavo A. R. Silva
2021-04-14 23:43 ` [PATCH v3 1/2] wl3501_cs: Fix out-of-bounds warnings in wl3501_send_pkt Gustavo A. R. Silva
@ 2021-04-14 23:45 ` Gustavo A. R. Silva
2021-04-15 19:58 ` Kees Cook
1 sibling, 1 reply; 7+ messages in thread
From: Gustavo A. R. Silva @ 2021-04-14 23:45 UTC (permalink / raw)
To: linux-kernel
Cc: Kalle Valo, David S. Miller, Jakub Kicinski, linux-wireless,
netdev, linux-hardening, Gustavo A. R. Silva, Kees Cook
Fix the following out-of-bounds warnings by adding a new structure
wl3501_req instead of duplicating the same members in structure
wl3501_join_req and wl3501_scan_confirm:
arch/x86/include/asm/string_32.h:182:25: warning: '__builtin_memcpy' offset [39, 108] from the object at 'sig' is out of the bounds of referenced subobject 'beacon_period' with type 'short unsigned int' at offset 36 [-Warray-bounds]
arch/x86/include/asm/string_32.h:182:25: warning: '__builtin_memcpy' offset [25, 95] from the object at 'sig' is out of the bounds of referenced subobject 'beacon_period' with type 'short unsigned int' at offset 22 [-Warray-bounds]
Refactor the code, accordingly:
$ pahole -C wl3501_req drivers/net/wireless/wl3501_cs.o
struct wl3501_req {
u16 beacon_period; /* 0 2 */
u16 dtim_period; /* 2 2 */
u16 cap_info; /* 4 2 */
u8 bss_type; /* 6 1 */
u8 bssid[6]; /* 7 6 */
struct iw_mgmt_essid_pset ssid; /* 13 34 */
struct iw_mgmt_ds_pset ds_pset; /* 47 3 */
struct iw_mgmt_cf_pset cf_pset; /* 50 8 */
struct iw_mgmt_ibss_pset ibss_pset; /* 58 4 */
struct iw_mgmt_data_rset bss_basic_rset; /* 62 10 */
/* size: 72, cachelines: 2, members: 10 */
/* last cacheline: 8 bytes */
};
$ pahole -C wl3501_join_req drivers/net/wireless/wl3501_cs.o
struct wl3501_join_req {
u16 next_blk; /* 0 2 */
u8 sig_id; /* 2 1 */
u8 reserved; /* 3 1 */
struct iw_mgmt_data_rset operational_rset; /* 4 10 */
u16 reserved2; /* 14 2 */
u16 timeout; /* 16 2 */
u16 probe_delay; /* 18 2 */
u8 timestamp[8]; /* 20 8 */
u8 local_time[8]; /* 28 8 */
struct wl3501_req req; /* 36 72 */
/* size: 108, cachelines: 2, members: 10 */
/* last cacheline: 44 bytes */
};
$ pahole -C wl3501_scan_confirm drivers/net/wireless/wl3501_cs.o
struct wl3501_scan_confirm {
u16 next_blk; /* 0 2 */
u8 sig_id; /* 2 1 */
u8 reserved; /* 3 1 */
u16 status; /* 4 2 */
char timestamp[8]; /* 6 8 */
char localtime[8]; /* 14 8 */
struct wl3501_req req; /* 22 72 */
/* --- cacheline 1 boundary (64 bytes) was 30 bytes ago --- */
u8 rssi; /* 94 1 */
/* size: 96, cachelines: 2, members: 8 */
/* padding: 1 */
/* last cacheline: 32 bytes */
};
The problem is that the original code is trying to copy data into a
bunch of struct members adjacent to each other in a single call to
memcpy(). Now that a new struct wl3501_req enclosing all those adjacent
members is introduced, memcpy() doesn't overrun the length of
&sig.beacon_period and &this->bss_set[i].beacon_period, because the
address of the new struct object _req_ is used as the destination,
instead.
This helps with the ongoing efforts to globally enable -Warray-bounds
and get us closer to being able to tighten the FORTIFY_SOURCE routines
on memcpy().
Link: https://github.com/KSPP/linux/issues/109
Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
---
Changes in v3:
- Add new struct wl3501_req and refactor the code, accordingly.
- Fix one more instance of this same issue.
- Update changelog text.
Changes in v2:
- None.
drivers/net/wireless/wl3501.h | 35 +++++++++++--------------
drivers/net/wireless/wl3501_cs.c | 44 +++++++++++++++++---------------
2 files changed, 38 insertions(+), 41 deletions(-)
diff --git a/drivers/net/wireless/wl3501.h b/drivers/net/wireless/wl3501.h
index aa8222cbea68..59b7b93c5963 100644
--- a/drivers/net/wireless/wl3501.h
+++ b/drivers/net/wireless/wl3501.h
@@ -379,16 +379,7 @@ struct wl3501_get_confirm {
u8 mib_value[100];
};
-struct wl3501_join_req {
- u16 next_blk;
- u8 sig_id;
- u8 reserved;
- struct iw_mgmt_data_rset operational_rset;
- u16 reserved2;
- u16 timeout;
- u16 probe_delay;
- u8 timestamp[8];
- u8 local_time[8];
+struct wl3501_req {
u16 beacon_period;
u16 dtim_period;
u16 cap_info;
@@ -401,6 +392,19 @@ struct wl3501_join_req {
struct iw_mgmt_data_rset bss_basic_rset;
};
+struct wl3501_join_req {
+ u16 next_blk;
+ u8 sig_id;
+ u8 reserved;
+ struct iw_mgmt_data_rset operational_rset;
+ u16 reserved2;
+ u16 timeout;
+ u16 probe_delay;
+ u8 timestamp[8];
+ u8 local_time[8];
+ struct wl3501_req req;
+};
+
struct wl3501_join_confirm {
u16 next_blk;
u8 sig_id;
@@ -443,16 +447,7 @@ struct wl3501_scan_confirm {
u16 status;
char timestamp[8];
char localtime[8];
- u16 beacon_period;
- u16 dtim_period;
- u16 cap_info;
- u8 bss_type;
- u8 bssid[ETH_ALEN];
- struct iw_mgmt_essid_pset ssid;
- struct iw_mgmt_ds_pset ds_pset;
- struct iw_mgmt_cf_pset cf_pset;
- struct iw_mgmt_ibss_pset ibss_pset;
- struct iw_mgmt_data_rset bss_basic_rset;
+ struct wl3501_req req;
u8 rssi;
};
diff --git a/drivers/net/wireless/wl3501_cs.c b/drivers/net/wireless/wl3501_cs.c
index 70307308635f..672f5d5f3f2c 100644
--- a/drivers/net/wireless/wl3501_cs.c
+++ b/drivers/net/wireless/wl3501_cs.c
@@ -590,7 +590,7 @@ static int wl3501_mgmt_join(struct wl3501_card *this, u16 stas)
struct wl3501_join_req sig = {
.sig_id = WL3501_SIG_JOIN_REQ,
.timeout = 10,
- .ds_pset = {
+ .req.ds_pset = {
.el = {
.id = IW_MGMT_INFO_ELEMENT_DS_PARAMETER_SET,
.len = 1,
@@ -599,7 +599,7 @@ static int wl3501_mgmt_join(struct wl3501_card *this, u16 stas)
},
};
- memcpy(&sig.beacon_period, &this->bss_set[stas].beacon_period, 72);
+ memcpy(&sig.req, &this->bss_set[stas].req, sizeof(sig.req));
return wl3501_esbq_exec(this, &sig, sizeof(sig));
}
@@ -667,35 +667,37 @@ static void wl3501_mgmt_scan_confirm(struct wl3501_card *this, u16 addr)
if (sig.status == WL3501_STATUS_SUCCESS) {
pr_debug("success");
if ((this->net_type == IW_MODE_INFRA &&
- (sig.cap_info & WL3501_MGMT_CAPABILITY_ESS)) ||
+ (sig.req.cap_info & WL3501_MGMT_CAPABILITY_ESS)) ||
(this->net_type == IW_MODE_ADHOC &&
- (sig.cap_info & WL3501_MGMT_CAPABILITY_IBSS)) ||
+ (sig.req.cap_info & WL3501_MGMT_CAPABILITY_IBSS)) ||
this->net_type == IW_MODE_AUTO) {
if (!this->essid.el.len)
matchflag = 1;
else if (this->essid.el.len == 3 &&
!memcmp(this->essid.essid, "ANY", 3))
matchflag = 1;
- else if (this->essid.el.len != sig.ssid.el.len)
+ else if (this->essid.el.len != sig.req.ssid.el.len)
matchflag = 0;
- else if (memcmp(this->essid.essid, sig.ssid.essid,
+ else if (memcmp(this->essid.essid, sig.req.ssid.essid,
this->essid.el.len))
matchflag = 0;
else
matchflag = 1;
if (matchflag) {
for (i = 0; i < this->bss_cnt; i++) {
- if (ether_addr_equal_unaligned(this->bss_set[i].bssid, sig.bssid)) {
+ if (ether_addr_equal_unaligned(this->bss_set[i].req.bssid,
+ sig.req.bssid)) {
matchflag = 0;
break;
}
}
}
if (matchflag && (i < 20)) {
- memcpy(&this->bss_set[i].beacon_period,
- &sig.beacon_period, 73);
+ memcpy(&this->bss_set[i].req,
+ &sig.req, sizeof(sig.req));
this->bss_cnt++;
this->rssi = sig.rssi;
+ this->bss_set[i].rssi = sig.rssi;
}
}
} else if (sig.status == WL3501_STATUS_TIMEOUT) {
@@ -887,19 +889,19 @@ static void wl3501_mgmt_join_confirm(struct net_device *dev, u16 addr)
if (this->join_sta_bss < this->bss_cnt) {
const int i = this->join_sta_bss;
memcpy(this->bssid,
- this->bss_set[i].bssid, ETH_ALEN);
- this->chan = this->bss_set[i].ds_pset.chan;
+ this->bss_set[i].req.bssid, ETH_ALEN);
+ this->chan = this->bss_set[i].req.ds_pset.chan;
iw_copy_mgmt_info_element(&this->keep_essid.el,
- &this->bss_set[i].ssid.el);
+ &this->bss_set[i].req.ssid.el);
wl3501_mgmt_auth(this);
}
} else {
const int i = this->join_sta_bss;
- memcpy(&this->bssid, &this->bss_set[i].bssid, ETH_ALEN);
- this->chan = this->bss_set[i].ds_pset.chan;
+ memcpy(&this->bssid, &this->bss_set[i].req.bssid, ETH_ALEN);
+ this->chan = this->bss_set[i].req.ds_pset.chan;
iw_copy_mgmt_info_element(&this->keep_essid.el,
- &this->bss_set[i].ssid.el);
+ &this->bss_set[i].req.ssid.el);
wl3501_online(dev);
}
} else {
@@ -1573,30 +1575,30 @@ static int wl3501_get_scan(struct net_device *dev, struct iw_request_info *info,
for (i = 0; i < this->bss_cnt; ++i) {
iwe.cmd = SIOCGIWAP;
iwe.u.ap_addr.sa_family = ARPHRD_ETHER;
- memcpy(iwe.u.ap_addr.sa_data, this->bss_set[i].bssid, ETH_ALEN);
+ memcpy(iwe.u.ap_addr.sa_data, this->bss_set[i].req.bssid, ETH_ALEN);
current_ev = iwe_stream_add_event(info, current_ev,
extra + IW_SCAN_MAX_DATA,
&iwe, IW_EV_ADDR_LEN);
iwe.cmd = SIOCGIWESSID;
iwe.u.data.flags = 1;
- iwe.u.data.length = this->bss_set[i].ssid.el.len;
+ iwe.u.data.length = this->bss_set[i].req.ssid.el.len;
current_ev = iwe_stream_add_point(info, current_ev,
extra + IW_SCAN_MAX_DATA,
&iwe,
- this->bss_set[i].ssid.essid);
+ this->bss_set[i].req.ssid.essid);
iwe.cmd = SIOCGIWMODE;
- iwe.u.mode = this->bss_set[i].bss_type;
+ iwe.u.mode = this->bss_set[i].req.bss_type;
current_ev = iwe_stream_add_event(info, current_ev,
extra + IW_SCAN_MAX_DATA,
&iwe, IW_EV_UINT_LEN);
iwe.cmd = SIOCGIWFREQ;
- iwe.u.freq.m = this->bss_set[i].ds_pset.chan;
+ iwe.u.freq.m = this->bss_set[i].req.ds_pset.chan;
iwe.u.freq.e = 0;
current_ev = iwe_stream_add_event(info, current_ev,
extra + IW_SCAN_MAX_DATA,
&iwe, IW_EV_FREQ_LEN);
iwe.cmd = SIOCGIWENCODE;
- if (this->bss_set[i].cap_info & WL3501_MGMT_CAPABILITY_PRIVACY)
+ if (this->bss_set[i].req.cap_info & WL3501_MGMT_CAPABILITY_PRIVACY)
iwe.u.data.flags = IW_ENCODE_ENABLED | IW_ENCODE_NOKEY;
else
iwe.u.data.flags = IW_ENCODE_DISABLED;
--
2.27.0
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v3 2/2] wl3501_cs: Fix out-of-bounds warnings in wl3501_mgmt_join
2021-04-14 23:45 ` [PATCH v3 2/2] wl3501_cs: Fix out-of-bounds warnings in wl3501_mgmt_join Gustavo A. R. Silva
@ 2021-04-15 19:58 ` Kees Cook
2021-04-15 20:59 ` Gustavo A. R. Silva
0 siblings, 1 reply; 7+ messages in thread
From: Kees Cook @ 2021-04-15 19:58 UTC (permalink / raw)
To: Gustavo A. R. Silva
Cc: linux-kernel, Kalle Valo, David S. Miller, Jakub Kicinski,
linux-wireless, netdev, linux-hardening
On Wed, Apr 14, 2021 at 06:45:15PM -0500, Gustavo A. R. Silva wrote:
> Fix the following out-of-bounds warnings by adding a new structure
> wl3501_req instead of duplicating the same members in structure
> wl3501_join_req and wl3501_scan_confirm:
>
> arch/x86/include/asm/string_32.h:182:25: warning: '__builtin_memcpy' offset [39, 108] from the object at 'sig' is out of the bounds of referenced subobject 'beacon_period' with type 'short unsigned int' at offset 36 [-Warray-bounds]
> arch/x86/include/asm/string_32.h:182:25: warning: '__builtin_memcpy' offset [25, 95] from the object at 'sig' is out of the bounds of referenced subobject 'beacon_period' with type 'short unsigned int' at offset 22 [-Warray-bounds]
>
> Refactor the code, accordingly:
>
> $ pahole -C wl3501_req drivers/net/wireless/wl3501_cs.o
> struct wl3501_req {
> u16 beacon_period; /* 0 2 */
> u16 dtim_period; /* 2 2 */
> u16 cap_info; /* 4 2 */
> u8 bss_type; /* 6 1 */
> u8 bssid[6]; /* 7 6 */
> struct iw_mgmt_essid_pset ssid; /* 13 34 */
> struct iw_mgmt_ds_pset ds_pset; /* 47 3 */
> struct iw_mgmt_cf_pset cf_pset; /* 50 8 */
> struct iw_mgmt_ibss_pset ibss_pset; /* 58 4 */
> struct iw_mgmt_data_rset bss_basic_rset; /* 62 10 */
>
> /* size: 72, cachelines: 2, members: 10 */
> /* last cacheline: 8 bytes */
> };
>
> $ pahole -C wl3501_join_req drivers/net/wireless/wl3501_cs.o
> struct wl3501_join_req {
> u16 next_blk; /* 0 2 */
> u8 sig_id; /* 2 1 */
> u8 reserved; /* 3 1 */
> struct iw_mgmt_data_rset operational_rset; /* 4 10 */
> u16 reserved2; /* 14 2 */
> u16 timeout; /* 16 2 */
> u16 probe_delay; /* 18 2 */
> u8 timestamp[8]; /* 20 8 */
> u8 local_time[8]; /* 28 8 */
> struct wl3501_req req; /* 36 72 */
>
> /* size: 108, cachelines: 2, members: 10 */
> /* last cacheline: 44 bytes */
> };
>
> $ pahole -C wl3501_scan_confirm drivers/net/wireless/wl3501_cs.o
> struct wl3501_scan_confirm {
> u16 next_blk; /* 0 2 */
> u8 sig_id; /* 2 1 */
> u8 reserved; /* 3 1 */
> u16 status; /* 4 2 */
> char timestamp[8]; /* 6 8 */
> char localtime[8]; /* 14 8 */
> struct wl3501_req req; /* 22 72 */
> /* --- cacheline 1 boundary (64 bytes) was 30 bytes ago --- */
> u8 rssi; /* 94 1 */
>
> /* size: 96, cachelines: 2, members: 8 */
> /* padding: 1 */
> /* last cacheline: 32 bytes */
> };
>
> The problem is that the original code is trying to copy data into a
> bunch of struct members adjacent to each other in a single call to
> memcpy(). Now that a new struct wl3501_req enclosing all those adjacent
> members is introduced, memcpy() doesn't overrun the length of
> &sig.beacon_period and &this->bss_set[i].beacon_period, because the
> address of the new struct object _req_ is used as the destination,
> instead.
>
> This helps with the ongoing efforts to globally enable -Warray-bounds
> and get us closer to being able to tighten the FORTIFY_SOURCE routines
> on memcpy().
>
> Link: https://github.com/KSPP/linux/issues/109
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
Awesome! Thank you for this solution.
Reviewed-by: Kees Cook <keescook@chromium.org>
--
Kees Cook
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3 2/2] wl3501_cs: Fix out-of-bounds warnings in wl3501_mgmt_join
2021-04-15 19:58 ` Kees Cook
@ 2021-04-15 20:59 ` Gustavo A. R. Silva
0 siblings, 0 replies; 7+ messages in thread
From: Gustavo A. R. Silva @ 2021-04-15 20:59 UTC (permalink / raw)
To: Kees Cook, Gustavo A. R. Silva
Cc: linux-kernel, Kalle Valo, David S. Miller, Jakub Kicinski,
linux-wireless, netdev, linux-hardening
On 4/15/21 14:58, Kees Cook wrote:
> On Wed, Apr 14, 2021 at 06:45:15PM -0500, Gustavo A. R. Silva wrote:
>> Fix the following out-of-bounds warnings by adding a new structure
>> wl3501_req instead of duplicating the same members in structure
>> wl3501_join_req and wl3501_scan_confirm:
>>
>> arch/x86/include/asm/string_32.h:182:25: warning: '__builtin_memcpy' offset [39, 108] from the object at 'sig' is out of the bounds of referenced subobject 'beacon_period' with type 'short unsigned int' at offset 36 [-Warray-bounds]
>> arch/x86/include/asm/string_32.h:182:25: warning: '__builtin_memcpy' offset [25, 95] from the object at 'sig' is out of the bounds of referenced subobject 'beacon_period' with type 'short unsigned int' at offset 22 [-Warray-bounds]
>>
>> Refactor the code, accordingly:
>>
>> $ pahole -C wl3501_req drivers/net/wireless/wl3501_cs.o
>> struct wl3501_req {
>> u16 beacon_period; /* 0 2 */
>> u16 dtim_period; /* 2 2 */
>> u16 cap_info; /* 4 2 */
>> u8 bss_type; /* 6 1 */
>> u8 bssid[6]; /* 7 6 */
>> struct iw_mgmt_essid_pset ssid; /* 13 34 */
>> struct iw_mgmt_ds_pset ds_pset; /* 47 3 */
>> struct iw_mgmt_cf_pset cf_pset; /* 50 8 */
>> struct iw_mgmt_ibss_pset ibss_pset; /* 58 4 */
>> struct iw_mgmt_data_rset bss_basic_rset; /* 62 10 */
>>
>> /* size: 72, cachelines: 2, members: 10 */
>> /* last cacheline: 8 bytes */
>> };
>>
>> $ pahole -C wl3501_join_req drivers/net/wireless/wl3501_cs.o
>> struct wl3501_join_req {
>> u16 next_blk; /* 0 2 */
>> u8 sig_id; /* 2 1 */
>> u8 reserved; /* 3 1 */
>> struct iw_mgmt_data_rset operational_rset; /* 4 10 */
>> u16 reserved2; /* 14 2 */
>> u16 timeout; /* 16 2 */
>> u16 probe_delay; /* 18 2 */
>> u8 timestamp[8]; /* 20 8 */
>> u8 local_time[8]; /* 28 8 */
>> struct wl3501_req req; /* 36 72 */
>>
>> /* size: 108, cachelines: 2, members: 10 */
>> /* last cacheline: 44 bytes */
>> };
>>
>> $ pahole -C wl3501_scan_confirm drivers/net/wireless/wl3501_cs.o
>> struct wl3501_scan_confirm {
>> u16 next_blk; /* 0 2 */
>> u8 sig_id; /* 2 1 */
>> u8 reserved; /* 3 1 */
>> u16 status; /* 4 2 */
>> char timestamp[8]; /* 6 8 */
>> char localtime[8]; /* 14 8 */
>> struct wl3501_req req; /* 22 72 */
>> /* --- cacheline 1 boundary (64 bytes) was 30 bytes ago --- */
>> u8 rssi; /* 94 1 */
>>
>> /* size: 96, cachelines: 2, members: 8 */
>> /* padding: 1 */
>> /* last cacheline: 32 bytes */
>> };
>>
>> The problem is that the original code is trying to copy data into a
>> bunch of struct members adjacent to each other in a single call to
>> memcpy(). Now that a new struct wl3501_req enclosing all those adjacent
>> members is introduced, memcpy() doesn't overrun the length of
>> &sig.beacon_period and &this->bss_set[i].beacon_period, because the
>> address of the new struct object _req_ is used as the destination,
>> instead.
>>
>> This helps with the ongoing efforts to globally enable -Warray-bounds
>> and get us closer to being able to tighten the FORTIFY_SOURCE routines
>> on memcpy().
>>
>> Link: https://github.com/KSPP/linux/issues/109
>> Reported-by: kernel test robot <lkp@intel.com>
>> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
>
> Awesome! Thank you for this solution.
>
> Reviewed-by: Kees Cook <keescook@chromium.org>
Thanks, Kees!
--
Gustavo
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3 1/2] wl3501_cs: Fix out-of-bounds warnings in wl3501_send_pkt
2021-04-14 23:43 ` [PATCH v3 1/2] wl3501_cs: Fix out-of-bounds warnings in wl3501_send_pkt Gustavo A. R. Silva
@ 2021-04-22 14:39 ` Kalle Valo
[not found] ` <20210422143910.C8B5CC4338A@smtp.codeaurora.org>
1 sibling, 0 replies; 7+ messages in thread
From: Kalle Valo @ 2021-04-22 14:39 UTC (permalink / raw)
To: Gustavo A. R. Silva
Cc: linux-kernel, David S. Miller, Jakub Kicinski, linux-wireless,
netdev, linux-hardening, Gustavo A. R. Silva, Kees Cook
"Gustavo A. R. Silva" <gustavoars@kernel.org> wrote:
> Fix the following out-of-bounds warnings by enclosing structure members
> daddr and saddr into new struct addr, in structures wl3501_md_req and
> wl3501_md_ind:
>
> arch/x86/include/asm/string_32.h:182:25: warning: '__builtin_memcpy' offset [18, 23] from the object at 'sig' is out of the bounds of referenced subobject 'daddr' with type 'u8[6]' {aka 'unsigned char[6]'} at offset 11 [-Warray-bounds]
> arch/x86/include/asm/string_32.h:182:25: warning: '__builtin_memcpy' offset [18, 23] from the object at 'sig' is out of the bounds of referenced subobject 'daddr' with type 'u8[6]' {aka 'unsigned char[6]'} at offset 11 [-Warray-bounds]
>
> Refactor the code, accordingly:
>
> $ pahole -C wl3501_md_req drivers/net/wireless/wl3501_cs.o
> struct wl3501_md_req {
> u16 next_blk; /* 0 2 */
> u8 sig_id; /* 2 1 */
> u8 routing; /* 3 1 */
> u16 data; /* 4 2 */
> u16 size; /* 6 2 */
> u8 pri; /* 8 1 */
> u8 service_class; /* 9 1 */
> struct {
> u8 daddr[6]; /* 10 6 */
> u8 saddr[6]; /* 16 6 */
> } addr; /* 10 12 */
>
> /* size: 22, cachelines: 1, members: 8 */
> /* last cacheline: 22 bytes */
> };
>
> $ pahole -C wl3501_md_ind drivers/net/wireless/wl3501_cs.o
> struct wl3501_md_ind {
> u16 next_blk; /* 0 2 */
> u8 sig_id; /* 2 1 */
> u8 routing; /* 3 1 */
> u16 data; /* 4 2 */
> u16 size; /* 6 2 */
> u8 reception; /* 8 1 */
> u8 pri; /* 9 1 */
> u8 service_class; /* 10 1 */
> struct {
> u8 daddr[6]; /* 11 6 */
> u8 saddr[6]; /* 17 6 */
> } addr; /* 11 12 */
>
> /* size: 24, cachelines: 1, members: 9 */
> /* padding: 1 */
> /* last cacheline: 24 bytes */
> };
>
> The problem is that the original code is trying to copy data into a
> couple of arrays adjacent to each other in a single call to memcpy().
> Now that a new struct _addr_ enclosing those two adjacent arrays
> is introduced, memcpy() doesn't overrun the length of &sig.daddr[0]
> and &sig.daddr, because the address of the new struct object _addr_
> is used, instead.
>
> This helps with the ongoing efforts to globally enable -Warray-bounds
> and get us closer to being able to tighten the FORTIFY_SOURCE routines
> on memcpy().
>
> Link: https://github.com/KSPP/linux/issues/109
> Reported-by: kernel test robot <lkp@intel.com>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
2 patches applied to wireless-drivers-next.git, thanks.
820aa37638a2 wl3501_cs: Fix out-of-bounds warnings in wl3501_send_pkt
bb43e5718d8f wl3501_cs: Fix out-of-bounds warnings in wl3501_mgmt_join
--
https://patchwork.kernel.org/project/linux-wireless/patch/d260fe56aed7112bff2be5b4d152d03ad7b78e78.1618442265.git.gustavoars@kernel.org/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3 1/2] wl3501_cs: Fix out-of-bounds warnings in wl3501_send_pkt
[not found] ` <20210422143910.C8B5CC4338A@smtp.codeaurora.org>
@ 2021-04-22 18:30 ` Gustavo A. R. Silva
0 siblings, 0 replies; 7+ messages in thread
From: Gustavo A. R. Silva @ 2021-04-22 18:30 UTC (permalink / raw)
To: Kalle Valo, Gustavo A. R. Silva
Cc: linux-kernel, David S. Miller, Jakub Kicinski, linux-wireless,
netdev, linux-hardening, Kees Cook
On 4/22/21 09:39, Kalle Valo wrote:
> 2 patches applied to wireless-drivers-next.git, thanks.
>
> 820aa37638a2 wl3501_cs: Fix out-of-bounds warnings in wl3501_send_pkt
> bb43e5718d8f wl3501_cs: Fix out-of-bounds warnings in wl3501_mgmt_join
Thanks, Kalle.
--
Gustavo
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-04-22 18:52 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-14 23:40 [PATCH v3 0/2] Fix out-of-bounds warnings Gustavo A. R. Silva
2021-04-14 23:43 ` [PATCH v3 1/2] wl3501_cs: Fix out-of-bounds warnings in wl3501_send_pkt Gustavo A. R. Silva
2021-04-22 14:39 ` Kalle Valo
[not found] ` <20210422143910.C8B5CC4338A@smtp.codeaurora.org>
2021-04-22 18:30 ` Gustavo A. R. Silva
2021-04-14 23:45 ` [PATCH v3 2/2] wl3501_cs: Fix out-of-bounds warnings in wl3501_mgmt_join Gustavo A. R. Silva
2021-04-15 19:58 ` Kees Cook
2021-04-15 20:59 ` Gustavo A. R. Silva
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.