* [PATCH v4] selinux: policydb - fix byte order and alignment issues
@ 2018-10-18 7:47 Ondrej Mosnacek
2018-10-18 15:16 ` William Roberts
2018-10-19 14:28 ` Stephen Smalley
0 siblings, 2 replies; 6+ messages in thread
From: Ondrej Mosnacek @ 2018-10-18 7:47 UTC (permalink / raw)
To: selinux, Paul Moore
Cc: Stephen Smalley, selinux, Ondrej Mosnacek, Daniel Jurgens,
Eli Cohen, James Morris, Doug Ledford, stable
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]);
+
+ 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);
--
2.17.2
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH v4] selinux: policydb - fix byte order and alignment issues
2018-10-18 7:47 [PATCH v4] selinux: policydb - fix byte order and alignment issues Ondrej Mosnacek
@ 2018-10-18 15:16 ` William Roberts
2018-10-19 14:28 ` Stephen Smalley
1 sibling, 0 replies; 6+ messages in thread
From: William Roberts @ 2018-10-18 15:16 UTC (permalink / raw)
To: omosnace
Cc: selinux, Paul Moore, stable, dledford, selinux, eli, Stephen Smalley
On Thu, Oct 18, 2018 at 5:57 AM Ondrej Mosnacek <omosnace@redhat.com> 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]);
> +
> + 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);
> --
> 2.17.2
>
LGTM
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v4] selinux: policydb - fix byte order and alignment issues
2018-10-18 7:47 [PATCH v4] selinux: policydb - fix byte order and alignment issues Ondrej Mosnacek
2018-10-18 15:16 ` William Roberts
@ 2018-10-19 14:28 ` Stephen Smalley
2018-10-20 1:04 ` William Roberts
2018-10-22 7:54 ` Ondrej Mosnacek
1 sibling, 2 replies; 6+ messages in thread
From: Stephen Smalley @ 2018-10-19 14:28 UTC (permalink / raw)
To: Ondrej Mosnacek, selinux, Paul Moore
Cc: selinux, Daniel Jurgens, Eli Cohen, James Morris, Doug Ledford, stable
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.
> +
> + 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);
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v4] selinux: policydb - fix byte order and alignment issues
2018-10-19 14:28 ` Stephen Smalley
@ 2018-10-20 1:04 ` William Roberts
2018-10-22 7:57 ` Ondrej Mosnacek
2018-10-22 7:54 ` Ondrej Mosnacek
1 sibling, 1 reply; 6+ messages in thread
From: William Roberts @ 2018-10-20 1:04 UTC (permalink / raw)
To: Stephen Smalley
Cc: omosnace, selinux, Paul Moore, stable, dledford, selinux, eli
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.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v4] selinux: policydb - fix byte order and alignment issues
2018-10-20 1:04 ` William Roberts
@ 2018-10-22 7:57 ` Ondrej Mosnacek
0 siblings, 0 replies; 6+ messages in thread
From: Ondrej Mosnacek @ 2018-10-22 7:57 UTC (permalink / raw)
To: William Roberts
Cc: Stephen Smalley, selinux, Paul Moore, stable, Doug Ledford,
SElinux list, Eli Cohen
On Sat, Oct 20, 2018 at 3:05 AM William Roberts
<bill.c.roberts@gmail.com> wrote:
> 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.
That's because there is no such check in libsepol... I guess it should
really be there, though. Let me add it in a separate 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.
--
Ondrej Mosnacek <omosnace at redhat dot com>
Associate Software Engineer, Security Technologies
Red Hat, Inc.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v4] selinux: policydb - fix byte order and alignment issues
2018-10-19 14:28 ` Stephen Smalley
2018-10-20 1:04 ` William Roberts
@ 2018-10-22 7:54 ` Ondrej Mosnacek
1 sibling, 0 replies; 6+ messages in thread
From: Ondrej Mosnacek @ 2018-10-22 7:54 UTC (permalink / raw)
To: Stephen Smalley
Cc: selinux, Paul Moore, SElinux list, Daniel Jurgens, Eli Cohen,
James Morris, Doug Ledford, stable
On Fri, Oct 19, 2018 at 4:26 PM 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.
Yup, that'll be another re-spin...
>
> > +
> > + 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);
> >
>
Thanks,
--
Ondrej Mosnacek <omosnace at redhat dot com>
Associate Software Engineer, Security Technologies
Red Hat, Inc.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-10-22 7:57 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-18 7:47 [PATCH v4] selinux: policydb - fix byte order and alignment issues Ondrej Mosnacek
2018-10-18 15:16 ` William Roberts
2018-10-19 14:28 ` Stephen Smalley
2018-10-20 1:04 ` William Roberts
2018-10-22 7:57 ` Ondrej Mosnacek
2018-10-22 7:54 ` Ondrej Mosnacek
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).