On Tue, Oct 16, 2018 at 4:19 PM Ondrej Mosnacek <omosnace@xxxxxxxxxx> wrote:
> On Tue, Oct 16, 2018 at 2:53 PM Stephen Smalley <sds@xxxxxxxxxxxxx> wrote:
> > On 10/16/2018 03:09 AM, Ondrej Mosnacek wrote:
> > > Add missing LE conversions to the Infiniband-related range checks. These
> > > were causing a failure to load any policy with an ibendportcon rule on
> > > BE systems. This can be reproduced by running:
> > >
> > > 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
> > >
> > > (On ppc64 it fails with "/sbin/load_policy: Can't load policy: Invalid
> > > argument")
> > >
> > > Also, the temporary buffers are only guaranteed to be aligned for 32-bit
> > > access so use (get/put)_unaligned_be64() for 64-bit accesses.
> > >
> > > 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@xxxxxxxxxxxx>
> > > Cc: Eli Cohen <eli@xxxxxxxxxxxx>
> > > Cc: James Morris <jmorris@xxxxxxxxx>
> > > Cc: Doug Ledford <dledford@xxxxxxxxxx>
> > > Cc: <stable@xxxxxxxxxxxxxxx> # 4.13+
> > > Fixes: a806f7a1616f ("selinux: Create policydb version for Infiniband support")
> > > Signed-off-by: Ondrej Mosnacek <omosnace@xxxxxxxxxx>
> > > ---
> > > security/selinux/ss/policydb.c | 28 +++++++++++++++-------------
> > > 1 file changed, 15 insertions(+), 13 deletions(-)
> > >
> > > 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..2b310e8f2923 100644
> > > --- a/security/selinux/ss/policydb.c
> > > +++ b/security/selinux/ss/policydb.c
> > > @@ -37,6 +37,7 @@
> > > #include <linux/errno.h>
> > > #include <linux/audit.h>
> > > #include <linux/flex_array.h>
> > > +#include <asm/unaligned.h>
> > > #include "security.h"
> > >
> > > #include "policydb.h"
> > > @@ -2108,7 +2109,7 @@ static int ocontext_read(struct policydb *p, struct policydb_compat_info *info,
> > > {
> > > int i, j, rc;
> > > u32 nel, len;
> > > - __le32 buf[3];
> > > + __le32 buf[4];
> > > struct ocontext *l, *c;
> > > u32 nodebuf[8];
> > >
> > > @@ -2218,20 +2219,20 @@ static int ocontext_read(struct policydb *p, struct policydb_compat_info *info,
> > > break;
> > > }
> > > case OCON_IBPKEY:
> > > - rc = next_entry(nodebuf, fp, sizeof(u32) * 4);
> > > + rc = next_entry(buf, fp, sizeof(u32) * 4);
> > > if (rc)
> > > goto out;
> > >
> > > - c->u.ibpkey.subnet_prefix = be64_to_cpu(*((__be64 *)nodebuf));
> > > + c->u.ibpkey.subnet_prefix = get_unaligned_be64(buf);
> > >
> > > - if (nodebuf[2] > 0xffff ||
> > > - nodebuf[3] > 0xffff) {
> > > + if (le32_to_cpu(buf[2]) > 0xffff ||
> > > + le32_to_cpu(buf[3]) > 0xffff) {
> > > 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 = le32_to_cpu(buf[2]);
> > > + c->u.ibpkey.high_pkey = le32_to_cpu(buf[3]);
> >
> > I'm wondering why the handling here is inconsistent with that of
> > OCON_NODE/OCON_NODE6, which also deals with network byte order / big
> > endian data.
>
> I believe OCON_NODE/OCON_NODE6 doesn't call be32_to_cpu() because the
> kernel code probably expects those values to be in the "network
> order", in the sense that when you call ntohl() (basically an alias
> for be32_to_cpu()) on them, then you get a value where the low bytes
> are actually in the low bits of the integer. There are comments that
> seem to be intended as a justification doing this. Perhaps the
> subnet_prefix has different semantics, perhaps not...
>
> > Also it is inconsistent with the corresponding userspace
> > code in libsepol for IBPKEY, which just does a memcpy() for copying
> > between the subnet_prefix and the buffer.
>
> Hm... indeed, the userspace code doesn't match here. Now noone really
> knows which of them has the intended format... this is a mess :/
After inspecting the code it seems that it is actually correct (both
in userspace and in the kernel). The kernel Infiniband driver gives
the subnet_prefix values to SELinux in the CPU order, so it converts
it when loading the policy and stores it in struct ocontext like that
(the path is: struct ocontext -> ... -> struct ib_port_attr, which has
it in CPU order as evidenced by this line [1]). The userspace, OTOH,
internally keeps its subnet_prefix values in the network order (i.e.
it does a direct memcpy() from the value returned in struct in6_addr
by inet_pton(3) [2]).
So, it is a bit confusing but I don't think the code needs semantic
changes here (though maybe it needs adding some comments...).
[1] https://elixir.bootlin.com/linux/v4.18.14/source/drivers/infiniband/core/device.c#L859
[2] https://github.com/SELinuxProject/selinux/blob/5e33a44c666b966de50121b2e93198d6da65d696/libsepol/src/ibpkey_record.c#L46
>
> >
> > Switching to buf entirely doesn't seem right since it is __le32 and the
> > first part is actually __be64.
>
> That's why I switched to using get/put_unaligned_be64() there. Now the
> first two elements are just treated as some eight bytes of memory, so
> it doesn't matter what type they are. The only issue with the
> unaligned accessors might be perfomance, but I don't think this part
> of code is that performance critical. Anyway, maybe I'm just trying
> too hard to avoid declaring a yet another buf there :)
>
> >
> > Maybe we ought to be splitting this into two next_entry() calls, one to
> > fetch the be64 subnet prefix into an appropriately aligned and typed
> > buffer and one to fetch the le32 low/high pkey values into buf?
>
> Yeah, that's probably the right way to do it, but I was too lazy to
> check what next_entry() does and I wasn't sure if splitting the call
> wouldn't break something.
>
> >
> > We also need to fix the libsepol code
> > (selinux/libsepol/src/policydb.c:ocontext_read_selinux) for the validity
> > check at least.
>
> Yes, there are similar bugs there as well...
>
> >
> > >
> > > rc = context_read_and_validate(&c->context[0],
> > > p,
> > > @@ -2249,7 +2250,8 @@ static int ocontext_read(struct policydb *p, struct policydb_compat_info *info,
> > > if (rc)
> > > goto out;
> > >
> > > - if (buf[1] > 0xff || buf[1] == 0) {
> > > + if (le32_to_cpu(buf[1]) > 0xff ||
> > > + le32_to_cpu(buf[1]) == 0) {
> > > rc = -EINVAL;
> > > goto out;
> > > }
> > > @@ -3105,7 +3107,7 @@ static int ocontext_write(struct policydb *p, struct policydb_compat_info *info,
> > > {
> > > unsigned int i, j, rc;
> > > size_t nel, len;
> > > - __le32 buf[3];
> > > + __le32 buf[4];
> > > u32 nodebuf[8];
> > > struct ocontext *c;
> > > for (i = 0; i < info->ocon_num; i++) {
> > > @@ -3192,12 +3194,12 @@ 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);
> > > + put_unaligned_be64(c->u.ibpkey.subnet_prefix, buf);
> > >
> > > - nodebuf[2] = cpu_to_le32(c->u.ibpkey.low_pkey);
> > > - nodebuf[3] = cpu_to_le32(c->u.ibpkey.high_pkey);
> > > + buf[2] = cpu_to_le32(c->u.ibpkey.low_pkey);
> > > + buf[3] = cpu_to_le32(c->u.ibpkey.high_pkey);
> > >
> > > - rc = put_entry(nodebuf, sizeof(u32), 4, fp);
> > > + rc = put_entry(buf, sizeof(u32), 4, fp);
> > > if (rc)
> > > return rc;
> > > rc = context_write(p, &c->context[0], fp);
> > >
> >
> > Likewise this doesn't seem consistent with the OCONTEXT_NODE/NODE6
> > handling here or the libsepol ocontext_write_selinux code for
> > OCON_IBPKEY. We could also split this into two put_entry() calls to
> > preserve the typing separation between the be64 and le32 data.
>
> See above.
>
> I will try to investigate the IBKEY inconsistency and fix it. In the
> meantime I should probably split off the checks fixes into a separate
> patch and re-send it (plus replicate them upstream). Stay tuned...
>
> --
> Ondrej Mosnacek <omosnace at redhat dot com>
> Associate Software Engineer, Security Technologies
> Red Hat, Inc.
--
Ondrej Mosnacek <omosnace at redhat dot com>
Associate Software Engineer, Security Technologies
Red Hat, Inc.
