netdev - RE: [net-next v5 3/3] net/tls: Remove redundant array allocation.

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:   Wed, 1 Aug 2018 13:49:32 +0000
From:   Vakul Garg <vakul.garg@....com>
To:     Dave Watson <davejwatson@...com>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "borisp@...lanox.com" <borisp@...lanox.com>,
        "aviadye@...lanox.com" <aviadye@...lanox.com>,
        Doron Roberts-Kedes <doronrk@...com>,
        David Miller <davem@...emloft.net>
Subject: RE: [net-next v5 3/3] net/tls: Remove redundant array allocation.



> -----Original Message-----
> From: Dave Watson [mailto:davejwatson@...com]
> Sent: Monday, July 23, 2018 10:05 PM
> To: David Miller <davem@...emloft.net>
> Cc: Vakul Garg <vakul.garg@....com>; netdev@...r.kernel.org;
> borisp@...lanox.com; aviadye@...lanox.com; Doron Roberts-Kedes
> <doronrk@...com>
> Subject: Re: [net-next v5 3/3] net/tls: Remove redundant array allocation.
> 
> On 07/21/18 07:25 PM, David Miller wrote:
> > From: Vakul Garg <vakul.garg@....com>
> > Date: Thu, 19 Jul 2018 21:56:13 +0530
> >
> > > In function decrypt_skb(), array allocation in case when sgout is
> > > NULL is unnecessary. Instead, local variable sgin_arr[] can be used.
> > >
> > > Signed-off-by: Vakul Garg <vakul.garg@....com>
> >
> > Hmmm...
> >
> > Dave, can you take a look at this?  Do you think there might have been
> > a reason you felt that you needed to dynamically allocate the
> > scatterlists when you COW and skb and do in-place decryption?
> >
> > I guess this change is ok, nsg can only get smaller when the SKB is
> > COW'd.
> >
> 
> > >         memcpy(iv, tls_ctx->rx.iv, TLS_CIPHER_AES_GCM_128_SALT_SIZE);
> > >         if (!sgout) {
> > >                 nsg = skb_cow_data(skb, 0, &unused) + 1;
> > > -               sgin = kmalloc_array(nsg, sizeof(*sgin), sk->sk_allocation);
> > >                 sgout = sgin;
> > >         }
> 
> I don't think this patch is safe as-is.  sgin_arr is a stack array of size
> MAX_SKB_FRAGS (+ overhead), while my read of skb_cow_data is that it
> walks the whole chain of skbs from skb->next, and can return any number of
> segments.  Therefore we need to heap allocate.  I think I copied the IPSEC
> code here.
> 
> For perf though, we could use the stack array if skb_cow_data returns <=
> MAX_SKB_FRAGS.

skb_cow_data() is being called only when caller passes sgout=NULL (i.e.
non-zero copy case). In case of zero-copy, we do not call skb_cow_data()
and just assume that MAX_SKB_FRAGS+2 sized scatterlist array sgin_arr[]
is sufficient. This assumption could be wrong. So skb_cow_data() should be
called unconditionally to determine number of scatterlist entries required
for skb.

> 
> This code is slightly confusing though, since we don't heap allocate in the
> zerocopy case - what happens is that skb_to_sgvec returns -EMSGSIZE, and
> we fall back to the non-zerocopy case, and return again to this function,
> where we then hit the skb_cow_data path and heap allocate.

If skb_to_sgvec return -EMSGSIZE, decrypt_skb() would return failure, 
resulting in abort of TLS session.