On Aug 29, 2016 16:56, "Paul Moore" <paul@xxxxxxxxxxxxxx> wrote:
>
> On Tue, Aug 23, 2016 at 4:49 PM, <william.c.roberts@xxxxxxxxx> wrote:
> > From: William Roberts <william.c.roberts@xxxxxxxxx>
> >
> > Throughout the SE Linux LSM, values taken from sepolicy are
> > used in places where length == 0 or length == <saturated>
> > matter, find and fix these.
> >
> > Signed-off-by: William Roberts <william.c.roberts@xxxxxxxxx>
> > ---
> > security/selinux/ss/conditional.c | 3 +++
> > security/selinux/ss/policydb.c | 4 ++++
> > security/selinux/ss/private.h | 7 +++++++
> > 3 files changed, 14 insertions(+)
> > create mode 100644 security/selinux/ss/private.h
> >
> > diff --git a/security/selinux/ss/conditional.c b/security/selinux/ss/conditional.c
> > index 456e1a9..ecc0fb6 100644
> > --- a/security/selinux/ss/conditional.c
> > +++ b/security/selinux/ss/conditional.c
> > @@ -16,6 +16,7 @@
> > #include "security.h"
> > #include "conditional.h"
> > #include "services.h"
> > +#include "private.h"
> >
> > /*
> > * cond_evaluate_expr evaluates a conditional expr
> > @@ -242,6 +243,8 @@ int cond_read_bool(struct policydb *p, struct hashtab *h, void *fp)
> > goto err;
> >
> > len = le32_to_cpu(buf[2]);
> > + if (zero_or_saturated(len))
> > + goto err;
> >
> > rc = -ENOMEM;
> > key = kmalloc(len + 1, GFP_KERNEL);
> > diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
> > index 4b24385..0e881f3 100644
> > --- a/security/selinux/ss/policydb.c
> > +++ b/security/selinux/ss/policydb.c
> > @@ -38,6 +38,7 @@
> > #include "conditional.h"
> > #include "mls.h"
> > #include "services.h"
> > +#include "private.h"
> >
> > #define _DEBUG_HASHES
> >
> > @@ -1094,6 +1095,9 @@ static int str_read(char **strp, gfp_t flags, void *fp, u32 len)
> > int rc;
> > char *str;
> >
> > + if (zero_or_saturated(len))
> > + return -EINVAL;
> > +
> > str = kmalloc(len + 1, flags);
> > if (!str)
> > return -ENOMEM;
> > diff --git a/security/selinux/ss/private.h b/security/selinux/ss/private.h
> > new file mode 100644
> > index 0000000..0e81a78
> > --- /dev/null
> > +++ b/security/selinux/ss/private.h
> > @@ -0,0 +1,7 @@
> > +#ifndef PRIVATE_H_
> > +#define PRIVATE_H_
> > +
> > +#define is_saturated(x) (x == (typeof(x))-1)
> > +#define zero_or_saturated(x) ((x == 0) || is_saturated(x))
> > +
> > +#endif
>
> While I'm not opposed to the idea of using a macro for this purpose,
> e.g. is_saturated() and zero_or_saturated(), I don't see much value in
> creating this macro buried in the SELinux directory. Especially if we
> end up creating a new header file just for these macros. Something as
> generic as this should be something we inherit from the generic kernel
> code so we can leverage existing conventions.
It made more sense when I wanted to keep the change similar to libsepol. But the str_read() function really reduced the number of checks. I ended up porting that to libsepol. With that said, the macro serves little purpose with the exception of readability.
>
> If you can find a macro like these in the core kernel code, go ahead
Ill look, doubt I'll find anything.
> and use it, otherwise please respin this with the bound checks open
> code
Sure
>
> Oh, and use "SELinux" ;)
>
> --
> paul moore
> www.paul-moore.com
> _______________________________________________
> Selinux mailing list
> Selinux@xxxxxxxxxxxxx
> To unsubscribe, send email to Selinux-leave@xxxxxxxxxxxxx.
> To get help, send an email containing "help" to Selinux-request@xxxxxxxxxxxxx.
_______________________________________________ Selinux mailing list Selinux@xxxxxxxxxxxxx To unsubscribe, send email to Selinux-leave@xxxxxxxxxxxxx. To get help, send an email containing "help" to Selinux-request@xxxxxxxxxxxxx.
