* [PATCH V3 1/3] brcmfmac: Avoid possible out-of-bounds read
@ 2017-09-17 4:08 Kevin Cernekee
2017-09-17 4:08 ` [PATCH V3 2/3] brcmfmac: Delete redundant length check Kevin Cernekee
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Kevin Cernekee @ 2017-09-17 4:08 UTC (permalink / raw)
To: arend.vanspriel, franky.lin
Cc: brcm80211-dev-list.pdl, linux-wireless, mnissler
In brcmf_p2p_notify_rx_mgmt_p2p_probereq(), chanspec is assigned before
the length of rxframe is validated. This could lead to uninitialized
data being accessed (but not printed). Since we already have a
perfectly good endian-swapped copy of rxframe->chanspec in ch.chspec,
and ch.chspec is not modified by decchspec(), avoid the extra
assignment and use ch.chspec in the debug print.
Suggested-by: Mattias Nissler <mnissler@chromium.org>
Signed-off-by: Kevin Cernekee <cernekee@chromium.org>
Reviewed-by: Arend van Spriel <arend.vanspriel@broadcom.com>
---
drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
V2->V3: No change
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c
index 2ce675ab40ef..1c450c0727cb 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c
@@ -1853,7 +1853,6 @@ s32 brcmf_p2p_notify_rx_mgmt_p2p_probereq(struct brcmf_if *ifp,
struct afx_hdl *afx_hdl = &p2p->afx_hdl;
struct brcmf_cfg80211_vif *vif = ifp->vif;
struct brcmf_rx_mgmt_data *rxframe = (struct brcmf_rx_mgmt_data *)data;
- u16 chanspec = be16_to_cpu(rxframe->chanspec);
struct brcmu_chan ch;
u8 *mgmt_frame;
u32 mgmt_frame_len;
@@ -1906,7 +1905,7 @@ s32 brcmf_p2p_notify_rx_mgmt_p2p_probereq(struct brcmf_if *ifp,
cfg80211_rx_mgmt(&vif->wdev, freq, 0, mgmt_frame, mgmt_frame_len, 0);
brcmf_dbg(INFO, "mgmt_frame_len (%d) , e->datalen (%d), chanspec (%04x), freq (%d)\n",
- mgmt_frame_len, e->datalen, chanspec, freq);
+ mgmt_frame_len, e->datalen, ch.chspec, freq);
return 0;
}
--
2.14.1.690.gbb1197296e-goog
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH V3 2/3] brcmfmac: Delete redundant length check
2017-09-17 4:08 [PATCH V3 1/3] brcmfmac: Avoid possible out-of-bounds read Kevin Cernekee
@ 2017-09-17 4:08 ` Kevin Cernekee
2017-09-17 4:08 ` [PATCH V3 3/3] brcmfmac: Add check for short event packets Kevin Cernekee
2017-10-02 14:07 ` [V3,1/3] brcmfmac: Avoid possible out-of-bounds read Kalle Valo
2 siblings, 0 replies; 6+ messages in thread
From: Kevin Cernekee @ 2017-09-17 4:08 UTC (permalink / raw)
To: arend.vanspriel, franky.lin
Cc: brcm80211-dev-list.pdl, linux-wireless, mnissler
brcmf_fweh_process_event() sets event->datalen to the
endian-swapped value of event_packet->msg.datalen, which is the
same as emsg.datalen. This length is already validated in
brcmf_fweh_process_event(), so there is no need to check it
again upon dequeuing the event.
Suggested-by: Arend van Spriel <arend.vanspriel@broadcom.com>
Signed-off-by: Kevin Cernekee <cernekee@chromium.org>
---
drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c | 5 -----
1 file changed, 5 deletions(-)
V2->V3: No change
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c
index 4eb1e1ce9ace..27e661fa356f 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c
@@ -257,11 +257,6 @@ static void brcmf_fweh_event_worker(struct work_struct *work)
brcmf_dbg_hex_dump(BRCMF_EVENT_ON(), event->data,
min_t(u32, emsg.datalen, 64),
"event payload, len=%d\n", emsg.datalen);
- if (emsg.datalen > event->datalen) {
- brcmf_err("event invalid length header=%d, msg=%d\n",
- event->datalen, emsg.datalen);
- goto event_free;
- }
/* special handling of interface event */
if (event->code == BRCMF_E_IF) {
--
2.14.1.690.gbb1197296e-goog
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH V3 3/3] brcmfmac: Add check for short event packets
2017-09-17 4:08 [PATCH V3 1/3] brcmfmac: Avoid possible out-of-bounds read Kevin Cernekee
2017-09-17 4:08 ` [PATCH V3 2/3] brcmfmac: Delete redundant length check Kevin Cernekee
@ 2017-09-17 4:08 ` Kevin Cernekee
2017-10-02 12:46 ` [V3,3/3] " Kalle Valo
2017-10-02 13:51 ` Kalle Valo
2017-10-02 14:07 ` [V3,1/3] brcmfmac: Avoid possible out-of-bounds read Kalle Valo
2 siblings, 2 replies; 6+ messages in thread
From: Kevin Cernekee @ 2017-09-17 4:08 UTC (permalink / raw)
To: arend.vanspriel, franky.lin
Cc: brcm80211-dev-list.pdl, linux-wireless, mnissler
The length of the data in the received skb is currently passed into
brcmf_fweh_process_event() as packet_len, but this value is not checked.
event_packet should be followed by DATALEN bytes of additional event
data. Ensure that the received packet actually contains at least
DATALEN bytes of additional data, to avoid copying uninitialized memory
into event->data.
Suggested-by: Mattias Nissler <mnissler@chromium.org>
Signed-off-by: Kevin Cernekee <cernekee@chromium.org>
---
drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
V2->V3: Change '<' to '>' and retest
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c
index 27e661fa356f..e7eaa57d11d9 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c
@@ -424,7 +424,8 @@ void brcmf_fweh_process_event(struct brcmf_pub *drvr,
if (code != BRCMF_E_IF && !fweh->evt_handler[code])
return;
- if (datalen > BRCMF_DCMD_MAXLEN)
+ if (datalen > BRCMF_DCMD_MAXLEN ||
+ datalen + sizeof(*event_packet) > packet_len)
return;
if (in_interrupt())
--
2.14.1.690.gbb1197296e-goog
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [V3,3/3] brcmfmac: Add check for short event packets
2017-09-17 4:08 ` [PATCH V3 3/3] brcmfmac: Add check for short event packets Kevin Cernekee
@ 2017-10-02 12:46 ` Kalle Valo
2017-10-02 13:51 ` Kalle Valo
1 sibling, 0 replies; 6+ messages in thread
From: Kalle Valo @ 2017-10-02 12:46 UTC (permalink / raw)
To: Kevin Cernekee
Cc: arend.vanspriel, franky.lin, brcm80211-dev-list.pdl,
linux-wireless, mnissler
Kevin Cernekee <cernekee@chromium.org> wrote:
> The length of the data in the received skb is currently passed into
> brcmf_fweh_process_event() as packet_len, but this value is not checked.
> event_packet should be followed by DATALEN bytes of additional event
> data. Ensure that the received packet actually contains at least
> DATALEN bytes of additional data, to avoid copying uninitialized memory
> into event->data.
>
> Suggested-by: Mattias Nissler <mnissler@chromium.org>
> Signed-off-by: Kevin Cernekee <cernekee@chromium.org>
I'll queue this for v4.14 and add:
Cc: stable@vger.kernel.org # v3.8
--
https://patchwork.kernel.org/patch/9954607/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [V3,3/3] brcmfmac: Add check for short event packets
2017-09-17 4:08 ` [PATCH V3 3/3] brcmfmac: Add check for short event packets Kevin Cernekee
2017-10-02 12:46 ` [V3,3/3] " Kalle Valo
@ 2017-10-02 13:51 ` Kalle Valo
1 sibling, 0 replies; 6+ messages in thread
From: Kalle Valo @ 2017-10-02 13:51 UTC (permalink / raw)
To: Kevin Cernekee
Cc: arend.vanspriel, franky.lin, brcm80211-dev-list.pdl,
linux-wireless, mnissler
Kevin Cernekee <cernekee@chromium.org> wrote:
> The length of the data in the received skb is currently passed into
> brcmf_fweh_process_event() as packet_len, but this value is not checked.
> event_packet should be followed by DATALEN bytes of additional event
> data. Ensure that the received packet actually contains at least
> DATALEN bytes of additional data, to avoid copying uninitialized memory
> into event->data.
>
> Cc: <stable@vger.kernel.org> # v3.8
> Suggested-by: Mattias Nissler <mnissler@chromium.org>
> Signed-off-by: Kevin Cernekee <cernekee@chromium.org>
Patch applied to wireless-drivers.git, thanks.
dd2349121bb1 brcmfmac: Add check for short event packets
--
https://patchwork.kernel.org/patch/9954607/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [V3,1/3] brcmfmac: Avoid possible out-of-bounds read
2017-09-17 4:08 [PATCH V3 1/3] brcmfmac: Avoid possible out-of-bounds read Kevin Cernekee
2017-09-17 4:08 ` [PATCH V3 2/3] brcmfmac: Delete redundant length check Kevin Cernekee
2017-09-17 4:08 ` [PATCH V3 3/3] brcmfmac: Add check for short event packets Kevin Cernekee
@ 2017-10-02 14:07 ` Kalle Valo
2 siblings, 0 replies; 6+ messages in thread
From: Kalle Valo @ 2017-10-02 14:07 UTC (permalink / raw)
To: Kevin Cernekee
Cc: arend.vanspriel, franky.lin, brcm80211-dev-list.pdl,
linux-wireless, mnissler
Kevin Cernekee <cernekee@chromium.org> wrote:
> In brcmf_p2p_notify_rx_mgmt_p2p_probereq(), chanspec is assigned before
> the length of rxframe is validated. This could lead to uninitialized
> data being accessed (but not printed). Since we already have a
> perfectly good endian-swapped copy of rxframe->chanspec in ch.chspec,
> and ch.chspec is not modified by decchspec(), avoid the extra
> assignment and use ch.chspec in the debug print.
>
> Suggested-by: Mattias Nissler <mnissler@chromium.org>
> Signed-off-by: Kevin Cernekee <cernekee@chromium.org>
> Reviewed-by: Arend van Spriel <arend.vanspriel@broadcom.com>
2 patches applied to wireless-drivers-next.git, thanks.
73f2c8e933b1 brcmfmac: Avoid possible out-of-bounds read
a7c9acc452b2 brcmfmac: Delete redundant length check
--
https://patchwork.kernel.org/patch/9954603/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-10-02 14:07 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-09-17 4:08 [PATCH V3 1/3] brcmfmac: Avoid possible out-of-bounds read Kevin Cernekee
2017-09-17 4:08 ` [PATCH V3 2/3] brcmfmac: Delete redundant length check Kevin Cernekee
2017-09-17 4:08 ` [PATCH V3 3/3] brcmfmac: Add check for short event packets Kevin Cernekee
2017-10-02 12:46 ` [V3,3/3] " Kalle Valo
2017-10-02 13:51 ` Kalle Valo
2017-10-02 14:07 ` [V3,1/3] brcmfmac: Avoid possible out-of-bounds read Kalle Valo
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.