netdev - Re: [PATCH net] mISDN: hfcpci: Fix a use after free in hfcmulti

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Thu, 22 Oct 2020 17:44:26 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     isdn@...ux-pingi.de
Cc:     Petr Mladek <pmladek@...e.com>,
        Sergey Senozhatsky <sergey.senozhatsky@...il.com>,
        Kefeng Wang <wangkefeng.wang@...wei.com>,
        netdev@...r.kernel.org, kernel-janitors@...r.kernel.org
Subject: Re: [PATCH net] mISDN: hfcpci: Fix a use after free in hfcmulti_tx()

On Thu, Oct 22, 2020 at 04:24:00PM +0200, isdn@...ux-pingi.de wrote:
> Hi Dan,
> 
> that looks wrong to me and never was a use after free.
> 
> sp is set either to the address containing the pointer to the actual
> D-channel SKB or to the actual B-channel SKB. This address is not freed
> and will not change in this context. The dev_kfree(*sp) will delete the
> old SKB and the call to  get_next_[bd]frame(), if returning true, will
> place a new SKB into this address, so (*sp) point to this new SKB.
> The len of course need to be the length of the new SKB, not the old one,
> which would be the result of this patch.
> 

Oh, wow.  You're absolutely right.  That's pretty subtle.  Thanks for
catching it.

regards,
dan carpenter


> Best regards
> Karsten
> 
> On 10/22/20 9:07 AM, Dan Carpenter wrote:
> > This frees "*sp" before dereferencing it to get "len = (*sp)->len;".
> > 
> > Fixes: af69fb3a8ffa ("Add mISDN HFC multiport driver")
> > Signed-off-by: Dan Carpenter <dan.carpenter@...cle.com>
> > ---
> >  drivers/isdn/hardware/mISDN/hfcmulti.c | 10 ++++------
> >  1 file changed, 4 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/isdn/hardware/mISDN/hfcmulti.c b/drivers/isdn/hardware/mISDN/hfcmulti.c
> > index 7013a3f08429..ce6c160e0df4 100644
> > --- a/drivers/isdn/hardware/mISDN/hfcmulti.c
> > +++ b/drivers/isdn/hardware/mISDN/hfcmulti.c
> > @@ -2152,16 +2152,14 @@ hfcmulti_tx(struct hfc_multi *hc, int ch)
> >  		HFC_wait_nodebug(hc);
> >  	}
> >  
> > +	len = (*sp)->len;
> >  	dev_kfree_skb(*sp);
> >  	/* check for next frame */
> > -	if (bch && get_next_bframe(bch)) {
> > -		len = (*sp)->len;
> > +	if (bch && get_next_bframe(bch))
> >  		goto next_frame;
> > -	}
> > -	if (dch && get_next_dframe(dch)) {
> > -		len = (*sp)->len;
> > +
> > +	if (dch && get_next_dframe(dch))
> >  		goto next_frame;
> > -	}
> >  
> >  	/*
> >  	 * now we have no more data, so in case of transparent,
> >