Driver was writing to skb header area without ensuring it was
writable i.e. uncloned.
Used skb_cow_header to ensure that header buffer is large enough
and is uncloned.
Detected when, in combination with the smsc85xx driver, both drivers
attempted to write different headers to the same header buffer.
Signed-off-by: James Hughes <[email protected]>
---
drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c | 15 +++++++++++++--
1 file changed, 13 insertions(+), 2 deletions(-)
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c
index 038a960..b9d7d08 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c
@@ -249,14 +249,19 @@ brcmf_proto_bcdc_set_dcmd(struct brcmf_pub *drvr, int ifidx, uint cmd,
return ret;
}
-static void
+static int
brcmf_proto_bcdc_hdrpush(struct brcmf_pub *drvr, int ifidx, u8 offset,
struct sk_buff *pktbuf)
{
struct brcmf_proto_bcdc_header *h;
+ int err;
brcmf_dbg(BCDC, "Enter\n");
+ err = skb_cow_head(pktbuf, BCDC_HEADER_LEN);
+ if (err)
+ return err;
+
/* Push BDC header used to convey priority for buses that don't */
skb_push(pktbuf, BCDC_HEADER_LEN);
@@ -271,6 +276,8 @@ brcmf_proto_bcdc_hdrpush(struct brcmf_pub *drvr, int ifidx, u8 offset,
h->data_offset = offset;
BCDC_SET_IF_IDX(h, ifidx);
trace_brcmf_bcdchdr(pktbuf->data);
+
+ return 0;
}
static int
@@ -330,7 +337,11 @@ static int
brcmf_proto_bcdc_txdata(struct brcmf_pub *drvr, int ifidx, u8 offset,
struct sk_buff *pktbuf)
{
- brcmf_proto_bcdc_hdrpush(drvr, ifidx, offset, pktbuf);
+ int err = brcmf_proto_bcdc_hdrpush(drvr, ifidx, offset, pktbuf);
+
+ if (err)
+ return err;
+
return brcmf_bus_txdata(drvr->bus_if, pktbuf);
}
--
2.9.3
On 4/20/2017 5:47 PM, James Hughes wrote:
> Driver was writing to skb header area without ensuring it was
> writable i.e. uncloned.
>
> Used skb_cow_header to ensure that header buffer is large enough
> and is uncloned.
>
> Detected when, in combination with the smsc85xx driver, both drivers
> attempted to write different headers to the same header buffer.
I guess this patch is limited to one file to address the comment from
Eric Dumazet. I prefer to talk through the initial patch you sent. Just
a procedural remark or two here. 1) Please drop 'brcm80211:' from the
Subject as the 'brcmfmac' prefix is sufficient, and 2) despite the
change of the Subject you should consider this version 2 of the initial
patch so the Subject should read '[PATCH V2] brcmfmac: ....'.
> Signed-off-by: James Hughes <[email protected]>
> ---
Related to remark 2) above you should provide a changelog here listing
what is changed compared to the initial patch.
Regards,
Arend
---
> drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c | 15 +++++++++++++--
> 1 file changed, 13 insertions(+), 2 deletions(-)
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c
> index 038a960..b9d7d08 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c
> @@ -249,14 +249,19 @@ brcmf_proto_bcdc_set_dcmd(struct brcmf_pub *drvr, int ifidx, uint cmd,
> return ret;
> }
>
> -static void
> +static int
> brcmf_proto_bcdc_hdrpush(struct brcmf_pub *drvr, int ifidx, u8 offset,
> struct sk_buff *pktbuf)
> {
> struct brcmf_proto_bcdc_header *h;
> + int err;
>
> brcmf_dbg(BCDC, "Enter\n");
>
> + err = skb_cow_head(pktbuf, BCDC_HEADER_LEN);
> + if (err)
> + return err;
> +
> /* Push BDC header used to convey priority for buses that don't */
> skb_push(pktbuf, BCDC_HEADER_LEN);
>
> @@ -271,6 +276,8 @@ brcmf_proto_bcdc_hdrpush(struct brcmf_pub *drvr, int ifidx, u8 offset,
> h->data_offset = offset;
> BCDC_SET_IF_IDX(h, ifidx);
> trace_brcmf_bcdchdr(pktbuf->data);
> +
> + return 0;
> }
>
> static int
> @@ -330,7 +337,11 @@ static int
> brcmf_proto_bcdc_txdata(struct brcmf_pub *drvr, int ifidx, u8 offset,
> struct sk_buff *pktbuf)
> {
> - brcmf_proto_bcdc_hdrpush(drvr, ifidx, offset, pktbuf);
> + int err = brcmf_proto_bcdc_hdrpush(drvr, ifidx, offset, pktbuf);
> +
> + if (err)
> + return err;
> +
> return brcmf_bus_txdata(drvr->bus_if, pktbuf);
> }
>
>
On 20 April 2017 at 20:22, Arend van Spriel
<[email protected]> wrote:
> On 4/20/2017 5:47 PM, James Hughes wrote:
>>
>> Driver was writing to skb header area without ensuring it was
>> writable i.e. uncloned.
>>
>> Used skb_cow_header to ensure that header buffer is large enough
>> and is uncloned.
>>
>> Detected when, in combination with the smsc85xx driver, both drivers
>> attempted to write different headers to the same header buffer.
>
>
> I guess this patch is limited to one file to address the comment from Eric
> Dumazet. I prefer to talk through the initial patch you sent. Just a
> procedural remark or two here. 1) Please drop 'brcm80211:' from the Subject
> as the 'brcmfmac' prefix is sufficient, and 2) despite the change of the
> Subject you should consider this version 2 of the initial patch so the
> Subject should read '[PATCH V2] brcmfmac: ....'.
>
>> Signed-off-by: James Hughes <[email protected]>
>> ---
>
> Related to remark 2) above you should provide a changelog here listing what
> is changed compared to the initial patch.
>
> Regards,
> Arend
>
> ---
>>
>> drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c | 15
>> +++++++++++++--
>> 1 file changed, 13 insertions(+), 2 deletions(-)
>
>
>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c
>> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c
>> index 038a960..b9d7d08 100644
>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c
>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c
>> @@ -249,14 +249,19 @@ brcmf_proto_bcdc_set_dcmd(struct brcmf_pub *drvr,
>> int ifidx, uint cmd,
>> return ret;
>> }
>> -static void
>> +static int
>> brcmf_proto_bcdc_hdrpush(struct brcmf_pub *drvr, int ifidx, u8 offset,
>> struct sk_buff *pktbuf)
>> {
>> struct brcmf_proto_bcdc_header *h;
>> + int err;
>> brcmf_dbg(BCDC, "Enter\n");
>> + err = skb_cow_head(pktbuf, BCDC_HEADER_LEN);
>> + if (err)
>> + return err;
>> +
>> /* Push BDC header used to convey priority for buses that don't */
>> skb_push(pktbuf, BCDC_HEADER_LEN);
>> @@ -271,6 +276,8 @@ brcmf_proto_bcdc_hdrpush(struct brcmf_pub *drvr, int
>> ifidx, u8 offset,
>> h->data_offset = offset;
>> BCDC_SET_IF_IDX(h, ifidx);
>> trace_brcmf_bcdchdr(pktbuf->data);
>> +
>> + return 0;
>> }
>> static int
>> @@ -330,7 +337,11 @@ static int
>> brcmf_proto_bcdc_txdata(struct brcmf_pub *drvr, int ifidx, u8 offset,
>> struct sk_buff *pktbuf)
>> {
>> - brcmf_proto_bcdc_hdrpush(drvr, ifidx, offset, pktbuf);
>> + int err = brcmf_proto_bcdc_hdrpush(drvr, ifidx, offset, pktbuf);
>> +
>> + if (err)
>> + return err;
>> +
>> return brcmf_bus_txdata(drvr->bus_if, pktbuf);
>> }
>>
This patch has been superseded, please ignore.