netdev - Re: [PATCH] xfrm: release device reference for invalid state

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:   Mon, 11 Nov 2019 12:32:35 +0100
From:   Steffen Klassert <steffen.klassert@...unet.com>
To:     Xiaodong Xu <stid.smth@...il.com>
CC:     <herbert@...dor.apana.org.au>, <davem@...emloft.net>,
        <chenborfc@....com>, <netdev@...r.kernel.org>
Subject: Re: [PATCH] xfrm: release device reference for invalid state

On Sun, Nov 10, 2019 at 10:38:41PM -0800, Xiaodong Xu wrote:
> Thanks for reviewing the patch, Steffen. Please check my replies below.
> 
> On Sun, Nov 10, 2019 at 10:17 PM Steffen Klassert
> <steffen.klassert@...unet.com> wrote:
> > > +++ b/net/xfrm/xfrm_input.c
> > > @@ -474,6 +474,13 @@ int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type)
> > >       if (encap_type < 0) {
> > >               x = xfrm_input_state(skb);
> > >
> > > +             /* An encap_type of -1 indicates async resumption. */
> > > +             if (encap_type == -1) {
> > > +                     async = 1;
> > > +                     seq = XFRM_SKB_CB(skb)->seq.input.low;
> > > +                     goto resume;
> > > +             }
> > > +
> > >               if (unlikely(x->km.state != XFRM_STATE_VALID)) {
> > >                       if (x->km.state == XFRM_STATE_ACQ)
> > >                               XFRM_INC_STATS(net, LINUX_MIB_XFRMACQUIREERROR);
> >
> > Why not just dropping the reference here if the state became invalid
> > after async resumption?
> >
> I was thinking about releasing the device reference immediately after
> checking the state in the async resumption too. However it seems more
> natural to me to simply jump to the 'resume' label in the async case.

If you add the check here, you add it to an error path. If you add the
check to the resume label, it is in the fastpath.