netdev - Re: [PATCH net-next 3/3] macsec: add brackets and indentation after calling macsec

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] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 1 Jul 2019 15:21:57 +0200
From:   Sabrina Dubroca <sd@...asysnail.net>
To:     Willem de Bruijn <willemdebruijn.kernel@...il.com>
Cc:     Andreas Steinmetz <ast@...dv.de>,
        Network Development <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next 3/3] macsec: add brackets and indentation after
 calling macsec_decrypt

2019-06-30, 22:05:41 -0400, Willem de Bruijn wrote:
> On Sun, Jun 30, 2019 at 4:48 PM Andreas Steinmetz <ast@...dv.de> wrote:
> >
> > At this point, skb could only be a valid pointer, so this patch does
> > not introduce any functional change.
> 
> Previously, macsec_post_decrypt could be called on the original skb if
> the initial condition was false and macsec_decrypt is skipped. That
> was probably unintended. Either way, then this is a functional change,
> and perhaps a bugfix?

Ouch, I missed that when Andreas sent me that patch before. No, it is
actually intended. If we skip macsec_decrypt(), we should still
account for that packet in the InPktsUnchecked/InPktsDelayed
counters. That's in Figure 10-5 in the standard.

Thanks for catching this, Willem. That patch should only move the
IS_ERR(skb) case under the block where macsec_decrypt() is called, but
not move the call to macsec_post_decrypt().


> > Signed-off-by: Andreas Steinmetz <ast@...dv.de>
> >
> > --- a/drivers/net/macsec.c      2019-06-30 22:05:17.785683634 +0200
> > +++ b/drivers/net/macsec.c      2019-06-30 22:05:20.526171178 +0200
> > @@ -1205,21 +1205,22 @@
> >
> >         /* Disabled && !changed text => skip validation */
> >         if (hdr->tci_an & MACSEC_TCI_C ||
> > -           secy->validate_frames != MACSEC_VALIDATE_DISABLED)
> > +           secy->validate_frames != MACSEC_VALIDATE_DISABLED) {
> >                 skb = macsec_decrypt(skb, dev, rx_sa, sci, secy);
> >
> > -       if (IS_ERR(skb)) {
> > -               /* the decrypt callback needs the reference */
> > -               if (PTR_ERR(skb) != -EINPROGRESS) {
> > -                       macsec_rxsa_put(rx_sa);
> > -                       macsec_rxsc_put(rx_sc);
> > +               if (IS_ERR(skb)) {
> > +                       /* the decrypt callback needs the reference */
> > +                       if (PTR_ERR(skb) != -EINPROGRESS) {
> > +                               macsec_rxsa_put(rx_sa);
> > +                               macsec_rxsc_put(rx_sc);
> > +                       }
> > +                       rcu_read_unlock();
> > +                       return RX_HANDLER_CONSUMED;
> >                 }
> > -               rcu_read_unlock();
> > -               return RX_HANDLER_CONSUMED;
> > -       }
> >
> > -       if (!macsec_post_decrypt(skb, secy, pn))
> > -               goto drop;
> > +               if (!macsec_post_decrypt(skb, secy, pn))
> > +                       goto drop;
> > +       }
> >
> >  deliver:
> >         macsec_finalize_skb(skb, secy->icv_len,
> >

-- 
Sabrina