netdev - Re: [PATCH net-next 1/3] macsec: remove superfluous function calls

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:   Tue, 2 Jul 2019 21:50:33 -0400
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     Willem de Bruijn <willemdebruijn.kernel@...il.com>
Cc:     Andreas Steinmetz <ast@...dv.de>,
        Network Development <netdev@...r.kernel.org>,
        Sabrina Dubroca <sd@...asysnail.net>
Subject: Re: [PATCH net-next 1/3] macsec: remove superfluous function calls

On Sun, Jun 30, 2019 at 9:57 PM Willem de Bruijn
<willemdebruijn.kernel@...il.com> wrote:
>
> On Sun, Jun 30, 2019 at 4:48 PM Andreas Steinmetz <ast@...dv.de> wrote:
> >
> > Remove superfluous skb_share_check() and skb_unshare().
> > macsec_decrypt is only called by macsec_handle_frame which
> > already does a skb_unshare().
>
> There is a subtle difference. skb_unshare() acts on cloned skbs, not
> shared skbs.
>
> It creates a private copy of data if clones may access it
> concurrently, which clearly is needed when modifying for decryption.
>
> At rx_handler, I don't think a shared skb happen (unlike clones, e.g.,
> from packet sockets). But it is peculiar that most, if not all,
> rx_handlers seem to test for it. That have started with the bridge
> device:
>
> commit 7b995651e373d6424f81db23f2ec503306dfd7f0
> Author: Herbert Xu <herbert@...dor.apana.org.au>
> Date:   Sun Oct 14 00:39:01 2007 -0700
>
>     [BRIDGE]: Unshare skb upon entry
>
>     Due to the special location of the bridging hook, it should never see a
>     shared packet anyway (certainly not with any in-kernel code).  So it
>     makes sense to unshare the skb there if necessary as that will greatly
>     simplify the code below it (in particular, netfilter).
>
> Anyway, remove the check only if certain.

Did you see this point? I notice the v2 without this mentioned. I
don't think you can remove an skb_share_check solely on the basis of a
previous skb_unshare. I agree that here a shared skb is unlikely, but
that is for a very different, less obvious, reason.




>
> >
> > Signed-off-by: Andreas Steinmetz <ast@...dv.de>
> >
> > --- a/drivers/net/macsec.c      2019-06-30 22:02:54.906908179 +0200
> > +++ b/drivers/net/macsec.c      2019-06-30 22:03:07.315785186 +0200
> > @@ -939,9 +939,6 @@
> >         u16 icv_len = secy->icv_len;
> >
> >         macsec_skb_cb(skb)->valid = false;
> > -       skb = skb_share_check(skb, GFP_ATOMIC);
> > -       if (!skb)
> > -               return ERR_PTR(-ENOMEM);
> >
> >         ret = skb_cow_data(skb, 0, &trailer);
> >         if (unlikely(ret < 0)) {
> > @@ -973,11 +970,6 @@
> >
> >                 aead_request_set_crypt(req, sg, sg, len, iv);
> >                 aead_request_set_ad(req, macsec_hdr_len(macsec_skb_cb(skb)->has_sci));
> > -               skb = skb_unshare(skb, GFP_ATOMIC);
> > -               if (!skb) {
> > -                       aead_request_free(req);
> > -                       return ERR_PTR(-ENOMEM);
> > -               }
> >         } else {
> >                 /* integrity only: all headers + data authenticated */
> >                 aead_request_set_crypt(req, sg, sg, icv_len, iv);
> >