linux-kernel - Re: [PATCH v2] general protection fault in sock_has

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]

Message-ID: <1517504530.1750.55.camel@tycho.nsa.gov>
Date:   Thu, 01 Feb 2018 12:02:10 -0500
From:   Stephen Smalley <sds@...ho.nsa.gov>
To:     Mark Salyzyn <salyzyn@...roid.com>,
        Paul Moore <paul@...l-moore.com>
Cc:     linux-kernel@...r.kernel.org,
        Paul Moore <paul@...uxfoundation.org>,
        Greg KH <gregkh@...uxfoundation.org>,
        Eric Dumazet <edumazet@...gle.com>, selinux@...ho.nsa.gov,
        linux-security-module@...r.kernel.org,
        Eric Paris <eparis@...isplace.org>,
        "Serge E . Hallyn" <serge@...lyn.com>,
        stable <stable@...r.kernel.org>, James Morris <jmorris@...ei.org>
Subject: Re: [PATCH v2] general protection fault in sock_has_perm

On Thu, 2018-02-01 at 08:20 -0800, Mark Salyzyn wrote:
> On 02/01/2018 08:00 AM, Paul Moore wrote:
> > On Thu, Feb 1, 2018 at 10:37 AM, Mark Salyzyn <salyzyn@...roid.com>
> > wrote:
> > > In the absence of commit a4298e4522d6 ("net: add SOCK_RCU_FREE
> > > socket
> > > flag") and all the associated infrastructure changes to take
> > > advantage
> > > of a RCU grace period before freeing, there is a heightened
> > > possibility that a security check is performed while an ill-timed
> > > setsockopt call races in from user space.  It then is prudent to
> > > null
> > > check sk_security, and if the case, reject the permissions.
> > > 
> > > . . .
> > > ---[ end trace 7b5aaf788fef6174 ]---
> > > 
> > > Signed-off-by: Mark Salyzyn <salyzyn@...roid.com>
> > > Signed-off-by: Paul Moore <paul@...uxfoundation.org>
> > 
> > No, in the previous thread I gave my ack, not my sign-off; please
> > be
> > more careful in the future.  It may seem silly, especially in this
> > particular case, but it is an important distinction when things
> > like
> > the DCO are concerned.
> > 
> > Anyway, here is my ack again.
> > 
> > Acked-by: Paul Moore <paul@...l-moore.com>
> > 
> 
> Ok, both Greg KH and yours should be considered Acked-By. Been 
> overstepping this boundary for _years_. AFAIK Signed-off-by is still 
> pending from Stephen Smalley <sds@...ho.nsa.gov> before this can roll
> in.
> 
> Lesson lurned

No, Paul's Acked-by is sufficient, and at most, I would only add
another Acked-by or Reviewed-by, not a Signed-off-by.  Signed-off-by is
only needed when one had something to do with the writing of the patch
or was in the path by which it was merged.

I don't object to this patch but I have a hard time adding another ack
because I don't truly understand the root cause or how this fixes it. 
Let's say sk_prot_free() calls security_sk_free() calls
selinux_sk_free_security() which sets sk->sk_security to NULL, and then
we proceed to free the sksec and then sk_prot_free() frees the sk
itself.  Now another sock is allocated (or perhaps a different object
altogether), reuses that memory, and whatever sk->sk_security happens
to contain is set to non-NULL.  We'll just blithely proceed past your
check and who knows what will happen from that point onward.