Return-path: Received: from mail-wm0-f68.google.com ([74.125.82.68]:33708 "EHLO mail-wm0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757213AbcAaJ4X (ORCPT ); Sun, 31 Jan 2016 04:56:23 -0500 Received: by mail-wm0-f68.google.com with SMTP id r129so4595308wmr.0 for ; Sun, 31 Jan 2016 01:56:22 -0800 (PST) Subject: Re: [PATCH FIX?] brcmfmac: fix possible overflows in flowrings code by bumping u8 to u16 To: =?UTF-8?B?UmFmYcWCIE1pxYJlY2tp?= , Kalle Valo , linux-wireless@vger.kernel.org References: <1454198830-13971-1-git-send-email-zajec5@gmail.com> Cc: Brett Rudley , Arend van Spriel , "Franky (Zhenhui) Lin" , Hante Meuleman , brcm80211-dev-list@broadcom.com From: Arend van Spriel Message-ID: <56ADDA44.90707@gmail.com> (sfid-20160131_105642_115902_4900FCAE) Date: Sun, 31 Jan 2016 10:56:20 +0100 MIME-Version: 1.0 In-Reply-To: <1454198830-13971-1-git-send-email-zajec5@gmail.com> Content-Type: text/plain; charset=utf-8 Sender: linux-wireless-owner@vger.kernel.org List-ID: On 31-01-16 01:07, Rafał Miłecki wrote: > Some devices may use more than 255 flowings, below is log from BCM4366: > [ 194.606245] brcmfmac: brcmf_pcie_init_ringbuffers Nr of flowrings is 264 > > At various places we were using u8 which could lead to storing wrong > number or infinite loops when indexing incorrectly. Initially this > issue was spotted as infinite loop in brcmf_flowring_detach. There has already been a patch submitted for this [1]. However, because you reported issues with it on your device (not sure which one). Did you test this patch on that particular device. I want Hante to review your patch, but indeed this would be 4.5 material and probably stable. Regards, Arend [1] http://thread.gmane.org/gmane.linux.kernel.wireless.general/141004/focus=141003 > Signed-off-by: Rafał Miłecki > --- > I guess it's a good candidate for a fix (4.5 material). Any objections? > --- > .../broadcom/brcm80211/brcmfmac/flowring.c | 24 +++++++++++----------- > .../broadcom/brcm80211/brcmfmac/flowring.h | 18 ++++++++-------- > .../wireless/broadcom/brcm80211/brcmfmac/msgbuf.c | 4 ++-- > .../wireless/broadcom/brcm80211/brcmfmac/msgbuf.h | 2 +- > 4 files changed, 24 insertions(+), 24 deletions(-) > > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/flowring.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/flowring.c > index 2ca783f..3d2373b 100644 > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/flowring.c > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/flowring.c > @@ -169,7 +169,7 @@ u32 brcmf_flowring_create(struct brcmf_flowring *flow, u8 da[ETH_ALEN], > } > > > -u8 brcmf_flowring_tid(struct brcmf_flowring *flow, u8 flowid) > +u8 brcmf_flowring_tid(struct brcmf_flowring *flow, u16 flowid) > { > struct brcmf_flowring_ring *ring; > > @@ -179,7 +179,7 @@ u8 brcmf_flowring_tid(struct brcmf_flowring *flow, u8 flowid) > } > > > -static void brcmf_flowring_block(struct brcmf_flowring *flow, u8 flowid, > +static void brcmf_flowring_block(struct brcmf_flowring *flow, u16 flowid, > bool blocked) > { > struct brcmf_flowring_ring *ring; > @@ -228,7 +228,7 @@ static void brcmf_flowring_block(struct brcmf_flowring *flow, u8 flowid, > } > > > -void brcmf_flowring_delete(struct brcmf_flowring *flow, u8 flowid) > +void brcmf_flowring_delete(struct brcmf_flowring *flow, u16 flowid) > { > struct brcmf_flowring_ring *ring; > u8 hash_idx; > @@ -253,7 +253,7 @@ void brcmf_flowring_delete(struct brcmf_flowring *flow, u8 flowid) > } > > > -u32 brcmf_flowring_enqueue(struct brcmf_flowring *flow, u8 flowid, > +u32 brcmf_flowring_enqueue(struct brcmf_flowring *flow, u16 flowid, > struct sk_buff *skb) > { > struct brcmf_flowring_ring *ring; > @@ -279,7 +279,7 @@ u32 brcmf_flowring_enqueue(struct brcmf_flowring *flow, u8 flowid, > } > > > -struct sk_buff *brcmf_flowring_dequeue(struct brcmf_flowring *flow, u8 flowid) > +struct sk_buff *brcmf_flowring_dequeue(struct brcmf_flowring *flow, u16 flowid) > { > struct brcmf_flowring_ring *ring; > struct sk_buff *skb; > @@ -300,7 +300,7 @@ struct sk_buff *brcmf_flowring_dequeue(struct brcmf_flowring *flow, u8 flowid) > } > > > -void brcmf_flowring_reinsert(struct brcmf_flowring *flow, u8 flowid, > +void brcmf_flowring_reinsert(struct brcmf_flowring *flow, u16 flowid, > struct sk_buff *skb) > { > struct brcmf_flowring_ring *ring; > @@ -311,7 +311,7 @@ void brcmf_flowring_reinsert(struct brcmf_flowring *flow, u8 flowid, > } > > > -u32 brcmf_flowring_qlen(struct brcmf_flowring *flow, u8 flowid) > +u32 brcmf_flowring_qlen(struct brcmf_flowring *flow, u16 flowid) > { > struct brcmf_flowring_ring *ring; > > @@ -326,7 +326,7 @@ u32 brcmf_flowring_qlen(struct brcmf_flowring *flow, u8 flowid) > } > > > -void brcmf_flowring_open(struct brcmf_flowring *flow, u8 flowid) > +void brcmf_flowring_open(struct brcmf_flowring *flow, u16 flowid) > { > struct brcmf_flowring_ring *ring; > > @@ -340,7 +340,7 @@ void brcmf_flowring_open(struct brcmf_flowring *flow, u8 flowid) > } > > > -u8 brcmf_flowring_ifidx_get(struct brcmf_flowring *flow, u8 flowid) > +u8 brcmf_flowring_ifidx_get(struct brcmf_flowring *flow, u16 flowid) > { > struct brcmf_flowring_ring *ring; > u8 hash_idx; > @@ -384,7 +384,7 @@ void brcmf_flowring_detach(struct brcmf_flowring *flow) > struct brcmf_pub *drvr = bus_if->drvr; > struct brcmf_flowring_tdls_entry *search; > struct brcmf_flowring_tdls_entry *remove; > - u8 flowid; > + u16 flowid; > > for (flowid = 0; flowid < flow->nrofrings; flowid++) { > if (flow->rings[flowid]) > @@ -408,7 +408,7 @@ void brcmf_flowring_configure_addr_mode(struct brcmf_flowring *flow, int ifidx, > struct brcmf_bus *bus_if = dev_get_drvdata(flow->dev); > struct brcmf_pub *drvr = bus_if->drvr; > u32 i; > - u8 flowid; > + u16 flowid; > > if (flow->addr_mode[ifidx] != addr_mode) { > for (i = 0; i < ARRAY_SIZE(flow->hash); i++) { > @@ -434,7 +434,7 @@ void brcmf_flowring_delete_peer(struct brcmf_flowring *flow, int ifidx, > struct brcmf_flowring_tdls_entry *prev; > struct brcmf_flowring_tdls_entry *search; > u32 i; > - u8 flowid; > + u16 flowid; > bool sta; > > sta = (flow->addr_mode[ifidx] == ADDR_INDIRECT); > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/flowring.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/flowring.h > index 95fd1c9..c59f684 100644 > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/flowring.h > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/flowring.h > @@ -24,7 +24,7 @@ struct brcmf_flowring_hash { > u8 mac[ETH_ALEN]; > u8 fifo; > u8 ifidx; > - u8 flowid; > + u16 flowid; > }; > > enum ring_status { > @@ -61,16 +61,16 @@ u32 brcmf_flowring_lookup(struct brcmf_flowring *flow, u8 da[ETH_ALEN], > u8 prio, u8 ifidx); > u32 brcmf_flowring_create(struct brcmf_flowring *flow, u8 da[ETH_ALEN], > u8 prio, u8 ifidx); > -void brcmf_flowring_delete(struct brcmf_flowring *flow, u8 flowid); > -void brcmf_flowring_open(struct brcmf_flowring *flow, u8 flowid); > -u8 brcmf_flowring_tid(struct brcmf_flowring *flow, u8 flowid); > -u32 brcmf_flowring_enqueue(struct brcmf_flowring *flow, u8 flowid, > +void brcmf_flowring_delete(struct brcmf_flowring *flow, u16 flowid); > +void brcmf_flowring_open(struct brcmf_flowring *flow, u16 flowid); > +u8 brcmf_flowring_tid(struct brcmf_flowring *flow, u16 flowid); > +u32 brcmf_flowring_enqueue(struct brcmf_flowring *flow, u16 flowid, > struct sk_buff *skb); > -struct sk_buff *brcmf_flowring_dequeue(struct brcmf_flowring *flow, u8 flowid); > -void brcmf_flowring_reinsert(struct brcmf_flowring *flow, u8 flowid, > +struct sk_buff *brcmf_flowring_dequeue(struct brcmf_flowring *flow, u16 flowid); > +void brcmf_flowring_reinsert(struct brcmf_flowring *flow, u16 flowid, > struct sk_buff *skb); > -u32 brcmf_flowring_qlen(struct brcmf_flowring *flow, u8 flowid); > -u8 brcmf_flowring_ifidx_get(struct brcmf_flowring *flow, u8 flowid); > +u32 brcmf_flowring_qlen(struct brcmf_flowring *flow, u16 flowid); > +u8 brcmf_flowring_ifidx_get(struct brcmf_flowring *flow, u16 flowid); > struct brcmf_flowring *brcmf_flowring_attach(struct device *dev, u16 nrofrings); > void brcmf_flowring_detach(struct brcmf_flowring *flow); > void brcmf_flowring_configure_addr_mode(struct brcmf_flowring *flow, int ifidx, > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/msgbuf.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/msgbuf.c > index c2bdb91..0b9c2dd 100644 > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/msgbuf.c > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/msgbuf.c > @@ -677,7 +677,7 @@ static u32 brcmf_msgbuf_flowring_create(struct brcmf_msgbuf *msgbuf, int ifidx, > } > > > -static void brcmf_msgbuf_txflow(struct brcmf_msgbuf *msgbuf, u8 flowid) > +static void brcmf_msgbuf_txflow(struct brcmf_msgbuf *msgbuf, u16 flowid) > { > struct brcmf_flowring *flow = msgbuf->flow; > struct brcmf_commonring *commonring; > @@ -1310,7 +1310,7 @@ int brcmf_proto_msgbuf_rx_trigger(struct device *dev) > } > > > -void brcmf_msgbuf_delete_flowring(struct brcmf_pub *drvr, u8 flowid) > +void brcmf_msgbuf_delete_flowring(struct brcmf_pub *drvr, u16 flowid) > { > struct brcmf_msgbuf *msgbuf = (struct brcmf_msgbuf *)drvr->proto->pd; > struct msgbuf_tx_flowring_delete_req *delete; > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/msgbuf.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/msgbuf.h > index 3d513e4..ee6906a 100644 > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/msgbuf.h > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/msgbuf.h > @@ -33,7 +33,7 @@ > > > int brcmf_proto_msgbuf_rx_trigger(struct device *dev); > -void brcmf_msgbuf_delete_flowring(struct brcmf_pub *drvr, u8 flowid); > +void brcmf_msgbuf_delete_flowring(struct brcmf_pub *drvr, u16 flowid); > int brcmf_proto_msgbuf_attach(struct brcmf_pub *drvr); > void brcmf_proto_msgbuf_detach(struct brcmf_pub *drvr); > #else >