On Fri, Dec 1, 2017 at 2:20 PM, Stephen Smalley <sds@xxxxxxxxxxxxx> wrote: > On Fri, 2017-12-01 at 10:34 -0500, Paul Moore wrote: >> On Thu, Nov 30, 2017 at 6:44 PM, William Roberts >> <bill.c.roberts@xxxxxxxxx> wrote: >> > On Thu, Nov 30, 2017 at 8:52 AM, Paul Moore <pmoore@xxxxxxxxxx> >> > wrote: >> > > From: Paul Moore <paul@xxxxxxxxxxxxxx> >> > > >> > > The syzbot/syzkaller automated tests found a problem in >> > > security_context_to_sid_core() during early boot (before we load >> > > the >> > > SELinux policy) where we could potentially feed context strings >> > > without >> > > NULL terminators into the strcmp() function. >> > > >> > > We already guard against this during normal operation (after the >> > > SELinux >> > > policy has been loaded) by making a copy of the context strings >> > > and >> > > explicitly adding a NULL terminator to the end. The patch >> > > extends this >> > > protection to the early boot case (no loaded policy) by moving >> > > the context >> > > copy earlier in security_context_to_sid_core(). >> > > >> > > Reported-by: syzbot <syzkaller@xxxxxxxxxxxxxxxx> >> > > Signed-off-by: Paul Moore <paul@xxxxxxxxxxxxxx> >> > > --- >> > > security/selinux/ss/services.c | 20 ++++++++++---------- >> > > 1 file changed, 10 insertions(+), 10 deletions(-) >> > > >> > > diff --git a/security/selinux/ss/services.c >> > > b/security/selinux/ss/services.c >> > > index 33cfe5d3d6cb..6ec4cdc8af8f 100644 >> > > --- a/security/selinux/ss/services.c >> > > +++ b/security/selinux/ss/services.c >> > > @@ -1413,27 +1413,27 @@ static int >> > > security_context_to_sid_core(const char *scontext, u32 >> > > scontext_len, >> > > if (!scontext_len) >> > > return -EINVAL; >> > > >> > > + /* Copy the string to allow changes and ensure a NULL >> > > terminator */ >> > > + scontext2 = kmalloc(scontext_len + 1, gfp_flags); >> > > + if (!scontext2) >> > > + return -ENOMEM; >> > > + memcpy(scontext2, scontext, scontext_len); >> > > + scontext2[scontext_len] = 0; >> > >> > Call me crazy, but can't we use kmemdup_nul() here? >> >> Crazy good idea ;) >> >> I didn't realize that function existed, I'll respin. Thanks. > > Also note that it should be NUL not NULL in the patch subject and > description. '\0' vs (void*)0 Yes, thanks. Muscle memory has me just typing "NULL" everywhere, regardless of the context. -- paul moore security @ redhat
