Re: [PATCH] brcm80211: brcmfmac: Ensure header writes are on writable skb

[Arend van Spriel]

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 <james.hug...@raspberrypi.org>
---
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);
  }