Re: [PATCH v4] selinux: policydb - fix byte order and alignment issues

[William Roberts <bill.c.roberts@gmail.com>]

On Fri, Oct 19, 2018 at 7:28 AM Stephen Smalley <sds@tycho.nsa.gov> wrote:
>
> On 10/18/2018 03:47 AM, Ondrej Mosnacek wrote:
> > Do the LE conversions before doing the Infiniband-related range checks.
> > The incorrect checks are otherwise causing a failure to load any policy
> > with an ibendportcon rule on BE systems. This can be reproduced by
> > running (on e.g. ppc64):
> >
> > cat >my_module.cil <<EOF
> > (type test_ibendport_t)
> > (roletype object_r test_ibendport_t)
> > (ibendportcon mlx4_0 1 (system_u object_r test_ibendport_t ((s0) (s0))))
> > EOF
> > semodule -i my_module.cil
> >
> > Also, fix loading/storing the 64-bit subnet prefix for OCON_IBPKEY to
> > use a correctly aligned buffer.
> >
> > Finally, do not use the 'nodebuf' (u32) buffer where 'buf' (__le32)
> > should be used instead.
> >
> > Tested internally on a ppc64 machine with a RHEL 7 kernel with this
> > patch applied.
> >
> > Cc: Daniel Jurgens <danielj@mellanox.com>
> > Cc: Eli Cohen <eli@mellanox.com>
> > Cc: James Morris <jmorris@namei.org>
> > Cc: Doug Ledford <dledford@redhat.com>
> > Cc: <stable@vger.kernel.org> # 4.13+
> > Fixes: a806f7a1616f ("selinux: Create policydb version for Infiniband support")
> > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> > ---
> >   security/selinux/ss/policydb.c | 46 +++++++++++++++++++++++-----------
> >   1 file changed, 32 insertions(+), 14 deletions(-)
> >
> > Changes in v4:
> >   - defer assignment to 16-bit struct fields to after the range check
> >
> > Changes in v3:
> >   - use separate buffer for the 64-bit subnet_prefix
> >   - add comments on the byte ordering of subnet_prefix
> >   - deduplicate the le32_to_cpu() calls from checks
> >
> > Changes in v2:
> >   - add reproducer to commit message
> >   - update e-mail address of James Morris
> >   - better Cc also the old SELinux ML
> >
> > diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
> > index f4eadd3f7350..d50668006a52 100644
> > --- a/security/selinux/ss/policydb.c
> > +++ b/security/selinux/ss/policydb.c
> > @@ -2108,6 +2108,7 @@ static int ocontext_read(struct policydb *p, struct policydb_compat_info *info,
> >   {
> >       int i, j, rc;
> >       u32 nel, len;
> > +     __be64 prefixbuf[1];
> >       __le32 buf[3];
> >       struct ocontext *l, *c;
> >       u32 nodebuf[8];
> > @@ -2217,21 +2218,30 @@ static int ocontext_read(struct policydb *p, struct policydb_compat_info *info,
> >                                       goto out;
> >                               break;
> >                       }
> > -                     case OCON_IBPKEY:
> > -                             rc = next_entry(nodebuf, fp, sizeof(u32) * 4);
> > +                     case OCON_IBPKEY: {
> > +                             u32 pkey_lo, pkey_hi;
> > +
> > +                             rc = next_entry(prefixbuf, fp, sizeof(u64));
> > +                             if (rc)
> > +                                     goto out;
> > +
> > +                             /* we need to have subnet_prefix in CPU order */
> > +                             c->u.ibpkey.subnet_prefix = be64_to_cpu(prefixbuf[0]);
> > +
> > +                             rc = next_entry(buf, fp, sizeof(u32) * 2);
> >                               if (rc)
> >                                       goto out;
> >
> > -                             c->u.ibpkey.subnet_prefix = be64_to_cpu(*((__be64 *)nodebuf));
> > +                             pkey_lo = le32_to_cpu(buf[0]);
> > +                             pkey_hi = le32_to_cpu(buf[1]);
> >
> > -                             if (nodebuf[2] > 0xffff ||
> > -                                 nodebuf[3] > 0xffff) {
> > +                             if (pkey_lo > U16_MAX || pkey_hi > U16_MAX) {
> >                                       rc = -EINVAL;
> >                                       goto out;
> >                               }
> >
> > -                             c->u.ibpkey.low_pkey = le32_to_cpu(nodebuf[2]);
> > -                             c->u.ibpkey.high_pkey = le32_to_cpu(nodebuf[3]);
> > +                             c->u.ibpkey.low_pkey  = pkey_lo;
> > +                             c->u.ibpkey.high_pkey = pkey_hi;
> >
> >                               rc = context_read_and_validate(&c->context[0],
> >                                                              p,
> > @@ -2239,6 +2249,7 @@ static int ocontext_read(struct policydb *p, struct policydb_compat_info *info,
> >                               if (rc)
> >                                       goto out;
> >                               break;
> > +                     }
> >                       case OCON_IBENDPORT:
> >                               rc = next_entry(buf, fp, sizeof(u32) * 2);
> >                               if (rc)
> > @@ -2249,13 +2260,14 @@ static int ocontext_read(struct policydb *p, struct policydb_compat_info *info,
> >                               if (rc)
> >                                       goto out;
> >
> > -                             if (buf[1] > 0xff || buf[1] == 0) {
> > +                             c->u.ibendport.port = le32_to_cpu(buf[1]);
>
> ibendport.port is a u8 here, same issue.

Good catch. Which made me notice this hunk is absent from the
userspace side and the userspace code has the same issue.
If we fix it up here in this patch we should be doing the same in the
libsepol patch.

>
> > +
> > +                             if (c->u.ibendport.port > 0xff ||
> > +                                 c->u.ibendport.port == 0) {
> >                                       rc = -EINVAL;
> >                                       goto out;
> >                               }
> >
> > -                             c->u.ibendport.port = le32_to_cpu(buf[1]);
> > -
> >                               rc = context_read_and_validate(&c->context[0],
> >                                                              p,
> >                                                              fp);
> > @@ -3105,6 +3117,7 @@ static int ocontext_write(struct policydb *p, struct policydb_compat_info *info,
> >   {
> >       unsigned int i, j, rc;
> >       size_t nel, len;
> > +     __be64 prefixbuf[1];
> >       __le32 buf[3];
> >       u32 nodebuf[8];
> >       struct ocontext *c;
> > @@ -3192,12 +3205,17 @@ static int ocontext_write(struct policydb *p, struct policydb_compat_info *info,
> >                                       return rc;
> >                               break;
> >                       case OCON_IBPKEY:
> > -                             *((__be64 *)nodebuf) = cpu_to_be64(c->u.ibpkey.subnet_prefix);
> > +                             /* subnet_prefix is in CPU order */
> > +                             prefixbuf[0] = cpu_to_be64(c->u.ibpkey.subnet_prefix);
> >
> > -                             nodebuf[2] = cpu_to_le32(c->u.ibpkey.low_pkey);
> > -                             nodebuf[3] = cpu_to_le32(c->u.ibpkey.high_pkey);
> > +                             rc = put_entry(prefixbuf, sizeof(u64), 1, fp);
> > +                             if (rc)
> > +                                     return rc;
> >
> > -                             rc = put_entry(nodebuf, sizeof(u32), 4, fp);
> > +                             buf[0] = cpu_to_le32(c->u.ibpkey.low_pkey);
> > +                             buf[1] = cpu_to_le32(c->u.ibpkey.high_pkey);
> > +
> > +                             rc = put_entry(buf, sizeof(u32), 2, fp);
> >                               if (rc)
> >                                       return rc;
> >                               rc = context_write(p, &c->context[0], fp);
> >
>
> _______________________________________________
> Selinux mailing list
> Selinux@tycho.nsa.gov
> To unsubscribe, send email to Selinux-leave@tycho.nsa.gov.
> To get help, send an email containing "help" to Selinux-request@tycho.nsa.gov.