diff mbox series

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

Message ID 20181018074704.23835-1-omosnace@redhat.com (mailing list archive)
State Superseded
Headers show
Series [v4] selinux: policydb - fix byte order and alignment issues | expand

Commit Message

Ondrej Mosnacek Oct. 18, 2018, 7:47 a.m. UTC 
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

Comments

William Roberts Oct. 18, 2018, 3:16 p.m. UTC | #1 
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
Stephen Smalley Oct. 19, 2018, 2:28 p.m. UTC | #2 
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);
>
William Roberts Oct. 20, 2018, 1:04 a.m. UTC | #3 
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.
Ondrej Mosnacek Oct. 22, 2018, 7:54 a.m. UTC | #4 
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 Oct. 22, 2018, 7:57 a.m. UTC | #5 
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.
diff mbox series

Patch

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