[v2] selinux: fix byte order and alignment issues in policydb.c

Commit Message

Ondrej Mosnacek Oct. 16, 2018, 7:09 a.m. UTC

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 <[e-mail]>
Cc: Eli Cohen <[e-mail]>
Cc: James Morris <[e-mail]>
Cc: Doug Ledford <[e-mail]>
Cc: <[e-mail]> # 4.13+
Fixes: a806f7a1616f ("selinux: Create policydb version for Infiniband support")
Signed-off-by: Ondrej Mosnacek <[e-mail]>
---
 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

Comments

Stephen Smalley Oct. 16, 2018, 12:55 p.m. UTC | #1

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 <[e-mail]>
> Cc: Eli Cohen <[e-mail]>
> Cc: James Morris <[e-mail]>
> Cc: Doug Ledford <[e-mail]>
> Cc: <[e-mail]> # 4.13+
> Fixes: a806f7a1616f ("selinux: Create policydb version for Infiniband support")
> Signed-off-by: Ondrej Mosnacek <[e-mail]>
> ---
>   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.  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.

Switching to buf entirely doesn't seem right since it is __le32 and the 
first part is actually __be64.

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?

We also need to fix the libsepol code 
(selinux/libsepol/src/policydb.c:ocontext_read_selinux) for the validity 
check at least.

>   
>   				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.

Ondrej Mosnacek Oct. 16, 2018, 2:19 p.m. UTC | #2

On Tue, Oct 16, 2018 at 2:53 PM Stephen Smalley <[e-mail]> 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 <[e-mail]>
> > Cc: Eli Cohen <[e-mail]>
> > Cc: James Morris <[e-mail]>
> > Cc: Doug Ledford <[e-mail]>
> > Cc: <[e-mail]> # 4.13+
> > Fixes: a806f7a1616f ("selinux: Create policydb version for Infiniband support")
> > Signed-off-by: Ondrej Mosnacek <[e-mail]>
> > ---
> >   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 :/

>
> 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 Oct. 17, 2018, 7:56 a.m. UTC | #3

On Tue, Oct 16, 2018 at 4:19 PM Ondrej Mosnacek <[e-mail]> wrote:
> On Tue, Oct 16, 2018 at 2:53 PM Stephen Smalley <[e-mail]> 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 <[e-mail]>
> > > Cc: Eli Cohen <[e-mail]>
> > > Cc: James Morris <[e-mail]>
> > > Cc: Doug Ledford <[e-mail]>
> > > Cc: <[e-mail]> # 4.13+
> > > Fixes: a806f7a1616f ("selinux: Create policydb version for Infiniband support")
> > > Signed-off-by: Ondrej Mosnacek <[e-mail]>
> > > ---
> > >   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.

Patch

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]);
 
 				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);