| Message ID | 20190409112011.19546-1-colin.king@canonical.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | brcmfmac: fix leak of mypkt on error return path | expand |
On Tue, Apr 09, 2019 at 12:20:11PM +0100, Colin King wrote: > From: Colin Ian King <colin.king@canonical.com> > > Currently if the call to brcmf_sdiod_set_backplane_window fails then > then error return path leaks mypkt. Fix this by returning by a new > error path labelled 'out' that calls brcmu_pkt_buf_free_skb to free > mypkt. > > Addresses-Coverity: ("Resource Leak") > Fixes: a7c3aa1509e2 ("brcmfmac: Remove brcmf_sdiod_addrprep()") > Signed-off-by: Colin Ian King <colin.king@canonical.com> > --- > drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c > index ec129864cc9c..f3d11e024e71 100644 > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c > @@ -628,7 +628,7 @@ int brcmf_sdiod_send_buf(struct brcmf_sdio_dev *sdiodev, u8 *buf, uint nbytes) > > err = brcmf_sdiod_set_backplane_window(sdiodev, addr); > if (err) > - return err; > + goto out; > > addr &= SBSDIO_SB_OFT_ADDR_MASK; > addr |= SBSDIO_SB_ACCESS_2_4B_FLAG; > @@ -636,7 +636,7 @@ int brcmf_sdiod_send_buf(struct brcmf_sdio_dev *sdiodev, u8 *buf, uint nbytes) > if (!err) ^^^^ This is not required, btw. > err = brcmf_sdiod_skbuff_write(sdiodev, sdiodev->func2, addr, > mypkt); > - > +out: > brcmu_pkt_buf_free_skb(mypkt); regards, dan carpenter
On 09/04/2019 12:32, Dan Carpenter wrote: > On Tue, Apr 09, 2019 at 12:20:11PM +0100, Colin King wrote: >> From: Colin Ian King <colin.king@canonical.com> >> >> Currently if the call to brcmf_sdiod_set_backplane_window fails then >> then error return path leaks mypkt. Fix this by returning by a new >> error path labelled 'out' that calls brcmu_pkt_buf_free_skb to free >> mypkt. >> >> Addresses-Coverity: ("Resource Leak") >> Fixes: a7c3aa1509e2 ("brcmfmac: Remove brcmf_sdiod_addrprep()") >> Signed-off-by: Colin Ian King <colin.king@canonical.com> >> --- >> drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c >> index ec129864cc9c..f3d11e024e71 100644 >> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c >> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c >> @@ -628,7 +628,7 @@ int brcmf_sdiod_send_buf(struct brcmf_sdio_dev *sdiodev, u8 *buf, uint nbytes) >> >> err = brcmf_sdiod_set_backplane_window(sdiodev, addr); >> if (err) >> - return err; >> + goto out; >> >> addr &= SBSDIO_SB_OFT_ADDR_MASK; >> addr |= SBSDIO_SB_ACCESS_2_4B_FLAG; >> @@ -636,7 +636,7 @@ int brcmf_sdiod_send_buf(struct brcmf_sdio_dev *sdiodev, u8 *buf, uint nbytes) >> if (!err) > ^^^^ > This is not required, btw. Good point. > >> err = brcmf_sdiod_skbuff_write(sdiodev, sdiodev->func2, addr, >> mypkt); >> - >> +out: >> brcmu_pkt_buf_free_skb(mypkt); > > regards, > dan carpenter >
On 4/9/2019 1:20 PM, Colin King wrote: > From: Colin Ian King <colin.king@canonical.com> > > Currently if the call to brcmf_sdiod_set_backplane_window fails then > then error return path leaks mypkt. Fix this by returning by a new > error path labelled 'out' that calls brcmu_pkt_buf_free_skb to free > mypkt. > > Addresses-Coverity: ("Resource Leak") > Fixes: a7c3aa1509e2 ("brcmfmac: Remove brcmf_sdiod_addrprep()") > Signed-off-by: Colin Ian King <colin.king@canonical.com> > --- > drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c > index ec129864cc9c..f3d11e024e71 100644 > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c > @@ -628,7 +628,7 @@ int brcmf_sdiod_send_buf(struct brcmf_sdio_dev *sdiodev, u8 *buf, uint nbytes) > > err = brcmf_sdiod_set_backplane_window(sdiodev, addr); > if (err) > - return err; > + goto out; Actually seems you can just get rid of the if statement entirely as below it skips brcmf_sdiod_skbuff_write() when an error occurred so no label is necessary. Regards, Arend > addr &= SBSDIO_SB_OFT_ADDR_MASK; > addr |= SBSDIO_SB_ACCESS_2_4B_FLAG; > @@ -636,7 +636,7 @@ int brcmf_sdiod_send_buf(struct brcmf_sdio_dev *sdiodev, u8 *buf, uint nbytes) > if (!err) > err = brcmf_sdiod_skbuff_write(sdiodev, sdiodev->func2, addr, > mypkt); > - > +out: > brcmu_pkt_buf_free_skb(mypkt); > > return err; >
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c index ec129864cc9c..f3d11e024e71 100644 --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c @@ -628,7 +628,7 @@ int brcmf_sdiod_send_buf(struct brcmf_sdio_dev *sdiodev, u8 *buf, uint nbytes) err = brcmf_sdiod_set_backplane_window(sdiodev, addr); if (err) - return err; + goto out; addr &= SBSDIO_SB_OFT_ADDR_MASK; addr |= SBSDIO_SB_ACCESS_2_4B_FLAG; @@ -636,7 +636,7 @@ int brcmf_sdiod_send_buf(struct brcmf_sdio_dev *sdiodev, u8 *buf, uint nbytes) if (!err) err = brcmf_sdiod_skbuff_write(sdiodev, sdiodev->func2, addr, mypkt); - +out: brcmu_pkt_buf_free_skb(mypkt); return err;