Re: [PATCH v3] selinux: policydb - fix byte order and alignment issues
On Wed, Oct 17, 2018 at 7:19 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 < (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 > Cc: Eli Cohen > Cc: James Morris > Cc: Doug Ledford > Cc: # 4.13+ > Fixes: a806f7a1616f ("selinux: Create policydb version for Infiniband > support") > Signed-off-by: Ondrej Mosnacek > --- > security/selinux/ss/policydb.c | 41 ++ > 1 file changed, 27 insertions(+), 14 deletions(-) > > 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..b9029491869b 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]; > @@ -2218,21 +2219,26 @@ 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(prefixbuf, fp, sizeof(u64)); > if (rc) > goto out; > > - c->u.ibpkey.subnet_prefix = > be64_to_cpu(*((__be64 *)nodebuf)); > + /* we need to have subnet_prefix in CPU order > */ > + c->u.ibpkey.subnet_prefix = > be64_to_cpu(prefixbuf[0]); > > - if (nodebuf[2] > 0x || > - nodebuf[3] > 0x) { > + rc = next_entry(buf, fp, sizeof(u32) * 2); > + if (rc) > + goto out; > + > + c->u.ibpkey.low_pkey = le32_to_cpu(buf[0]); > + c->u.ibpkey.high_pkey = le32_to_cpu(buf[1]); We noticed this in the coresponding user space patch. Assigning 32 to 16 truncates the values and thus the conditionals below are always false. struct { u64 subnet_prefix; u16 low_pkey; <-- u16 u16 high_pkey; <-- u16 } ibpkey; I figured I would comment just to make sure folks following one patch and not the other are informed. You'll need to respin a v4 and asign to a u32 intermediate, range check and the assign. > + > + if (c->u.ibpkey.low_pkey > 0x || > + c->u.ibpkey.high_pkey > 0x) { > rc = -EINVAL; > goto out; > } > > - c->u.ibpkey.low_pkey = > le32_to_cpu(nodebuf[2]); > - c->u.ibpkey.high_pkey = > le32_to_cpu(nodebuf[3]); > - > rc = context_read_and_validate(>context[0], >p, >fp); > @@ -2249,13 +2255,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]); > - >
Re: [PATCH v3] selinux: policydb - fix byte order and alignment issues
On 10/17/2018 10:15 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 < Cc: Eli Cohen Cc: James Morris Cc: Doug Ledford Cc: # 4.13+ Fixes: a806f7a1616f ("selinux: Create policydb version for Infiniband support") Signed-off-by: Ondrej Mosnacek Acked-by: Stephen Smalley --- security/selinux/ss/policydb.c | 41 ++ 1 file changed, 27 insertions(+), 14 deletions(-) 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..b9029491869b 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]; @@ -2218,21 +2219,26 @@ 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(prefixbuf, fp, sizeof(u64)); if (rc) goto out; -c->u.ibpkey.subnet_prefix = be64_to_cpu(*((__be64 *)nodebuf)); + /* we need to have subnet_prefix in CPU order */ + c->u.ibpkey.subnet_prefix = be64_to_cpu(prefixbuf[0]); -if (nodebuf[2] > 0x || - nodebuf[3] > 0x) { + rc = next_entry(buf, fp, sizeof(u32) * 2); + if (rc) + goto out; + + c->u.ibpkey.low_pkey = le32_to_cpu(buf[0]); + c->u.ibpkey.high_pkey = le32_to_cpu(buf[1]); + + if (c->u.ibpkey.low_pkey > 0x || + c->u.ibpkey.high_pkey > 0x) { rc = -EINVAL; goto out; } -c->u.ibpkey.low_pkey = le32_to_cpu(nodebuf[2]); - c->u.ibpkey.high_pkey = le32_to_cpu(nodebuf[3]); - rc = context_read_and_validate(>context[0], p, fp); @@ -2249,13 +2255,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(>context[0], p, fp); @@ -3105,6 +3112,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 +3200,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); +
[PATCH v3] selinux: policydb - fix byte order and alignment issues
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 < Cc: Eli Cohen Cc: James Morris Cc: Doug Ledford Cc: # 4.13+ Fixes: a806f7a1616f ("selinux: Create policydb version for Infiniband support") Signed-off-by: Ondrej Mosnacek --- security/selinux/ss/policydb.c | 41 ++ 1 file changed, 27 insertions(+), 14 deletions(-) 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..b9029491869b 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]; @@ -2218,21 +2219,26 @@ 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(prefixbuf, fp, sizeof(u64)); if (rc) goto out; - c->u.ibpkey.subnet_prefix = be64_to_cpu(*((__be64 *)nodebuf)); + /* we need to have subnet_prefix in CPU order */ + c->u.ibpkey.subnet_prefix = be64_to_cpu(prefixbuf[0]); - if (nodebuf[2] > 0x || - nodebuf[3] > 0x) { + rc = next_entry(buf, fp, sizeof(u32) * 2); + if (rc) + goto out; + + c->u.ibpkey.low_pkey = le32_to_cpu(buf[0]); + c->u.ibpkey.high_pkey = le32_to_cpu(buf[1]); + + if (c->u.ibpkey.low_pkey > 0x || + c->u.ibpkey.high_pkey > 0x) { rc = -EINVAL; goto out; } - c->u.ibpkey.low_pkey = le32_to_cpu(nodebuf[2]); - c->u.ibpkey.high_pkey = le32_to_cpu(nodebuf[3]); - rc = context_read_and_validate(>context[0], p, fp); @@ -2249,13 +2255,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(>context[0], p, fp); @@ -3105,6 +3112,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 +3200,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 =