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] libsepol: fix endianity in ibpkey range checks
On Wed, Oct 17, 2018 at 2:21 PM Stephen Smalley wrote: > > On 10/17/2018 05:18 PM, Paul Moore wrote: > > On Wed, Oct 17, 2018 at 12:07 PM William Roberts > > wrote: > >> On Wed, Oct 17, 2018 at 7:48 AM Ondrej Mosnacek > >> wrote: > >>> > >>> We need to convert from little-endian before dong range checks on the > >>> ibpkey port numbers, otherwise we would be checking a wrong value. > >>> > >>> Fixes: 9fbb3112769a ("libsepol: Add ibpkey ocontext handling") > >>> Signed-off-by: Ondrej Mosnacek > >>> --- > >>> libsepol/src/policydb.c | 14 ++ > >>> 1 file changed, 10 insertions(+), 4 deletions(-) > >>> > >>> diff --git a/libsepol/src/policydb.c b/libsepol/src/policydb.c > >>> index a6d76ca3..dc201e2f 100644 > >>> --- a/libsepol/src/policydb.c > >>> +++ b/libsepol/src/policydb.c > >>> @@ -2830,15 +2830,21 @@ static int ocontext_read_selinux(struct > >>> policydb_compat_info *info, > >>> break; > >>> case OCON_IBPKEY: > >>> rc = next_entry(buf, fp, > >>> sizeof(uint32_t) * 4); > >>> - if (rc < 0 || buf[2] > 0x || buf[3] > > >>> 0x) > >>> + if (rc < 0) > >>> return -1; > >>> > >>> + c->u.ibpkey.low_pkey = > >>> le32_to_cpu(buf[2]); > >>> + c->u.ibpkey.high_pkey = > >>> le32_to_cpu(buf[3]); > >> > >> Ahh you're assigning a 32 bit value to a 16 bit variable low|high_pkey. I > >> think > >> you need to assign them to a uint32_t if you want to actually range check > >> them. > > > > If you can, give me a chance to look over the kernel changes first. I > > doubt I'll see anything objectionable given the review the patches > > have already seen, but one never knows. > > The kernel patch has the same bug, so it will also need a re-spin. Good > catch. Don't thank me, thank GCC and Travis. This compiled on my local machine and ran the test suite just fine. I had clang set up, I guess this re-iterates the need and benefit of Travis testing in different environments. > > > > >>> + > >>> + if (c->u.ibpkey.low_pkey > 0x || > >>> + c->u.ibpkey.high_pkey > 0x) > >>> + return -1; > >>> + > >>> + /* we want c->u.ibpkey.subnet_prefix in > >>> network > >>> +* (big-endian) order, just memcpy it */ > >>> memcpy(>u.ibpkey.subnet_prefix, buf, > >>> > >>> sizeof(c->u.ibpkey.subnet_prefix)); > >>> > >>> - c->u.ibpkey.low_pkey = > >>> le32_to_cpu(buf[2]); > >>> - c->u.ibpkey.high_pkey = > >>> le32_to_cpu(buf[3]); > >>> - > >>> if (context_read_and_validate > >>> (>context[0], p, fp)) > >>> return -1; > >>> -- > >>> 2.17.2 > >>> > >> See job: https://travis-ci.org/SELinuxProject/selinux/jobs/442750208 > >> > >> Build fail with gcc: > >> > >> policydb.c:2839:31: error: comparison is always false due to limited > >> range of data type [-Werror=type-limits] > >> if (c->u.ibpkey.low_pkey > 0x || > >> ^ > >> policydb.c:2840:31: error: comparison is always false due to limited > >> range of data type [-Werror=type-limits] > >> c->u.ibpkey.high_pkey > 0x) > > > > > > > ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH] libsepol: fix endianity in ibpkey range checks
On 10/17/2018 05:18 PM, Paul Moore wrote: On Wed, Oct 17, 2018 at 12:07 PM William Roberts wrote: On Wed, Oct 17, 2018 at 7:48 AM Ondrej Mosnacek wrote: We need to convert from little-endian before dong range checks on the ibpkey port numbers, otherwise we would be checking a wrong value. Fixes: 9fbb3112769a ("libsepol: Add ibpkey ocontext handling") Signed-off-by: Ondrej Mosnacek --- libsepol/src/policydb.c | 14 ++ 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/libsepol/src/policydb.c b/libsepol/src/policydb.c index a6d76ca3..dc201e2f 100644 --- a/libsepol/src/policydb.c +++ b/libsepol/src/policydb.c @@ -2830,15 +2830,21 @@ static int ocontext_read_selinux(struct policydb_compat_info *info, break; case OCON_IBPKEY: rc = next_entry(buf, fp, sizeof(uint32_t) * 4); - if (rc < 0 || buf[2] > 0x || buf[3] > 0x) + if (rc < 0) return -1; + c->u.ibpkey.low_pkey = le32_to_cpu(buf[2]); + c->u.ibpkey.high_pkey = le32_to_cpu(buf[3]); Ahh you're assigning a 32 bit value to a 16 bit variable low|high_pkey. I think you need to assign them to a uint32_t if you want to actually range check them. If you can, give me a chance to look over the kernel changes first. I doubt I'll see anything objectionable given the review the patches have already seen, but one never knows. The kernel patch has the same bug, so it will also need a re-spin. Good catch. + + if (c->u.ibpkey.low_pkey > 0x || + c->u.ibpkey.high_pkey > 0x) + return -1; + + /* we want c->u.ibpkey.subnet_prefix in network +* (big-endian) order, just memcpy it */ memcpy(>u.ibpkey.subnet_prefix, buf, sizeof(c->u.ibpkey.subnet_prefix)); - c->u.ibpkey.low_pkey = le32_to_cpu(buf[2]); - c->u.ibpkey.high_pkey = le32_to_cpu(buf[3]); - if (context_read_and_validate (>context[0], p, fp)) return -1; -- 2.17.2 See job: https://travis-ci.org/SELinuxProject/selinux/jobs/442750208 Build fail with gcc: policydb.c:2839:31: error: comparison is always false due to limited range of data type [-Werror=type-limits] if (c->u.ibpkey.low_pkey > 0x || ^ policydb.c:2840:31: error: comparison is always false due to limited range of data type [-Werror=type-limits] c->u.ibpkey.high_pkey > 0x) ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH] libsepol: fix endianity in ibpkey range checks
On Wed, Oct 17, 2018 at 12:07 PM William Roberts wrote: > On Wed, Oct 17, 2018 at 7:48 AM Ondrej Mosnacek wrote: > > > > We need to convert from little-endian before dong range checks on the > > ibpkey port numbers, otherwise we would be checking a wrong value. > > > > Fixes: 9fbb3112769a ("libsepol: Add ibpkey ocontext handling") > > Signed-off-by: Ondrej Mosnacek > > --- > > libsepol/src/policydb.c | 14 ++ > > 1 file changed, 10 insertions(+), 4 deletions(-) > > > > diff --git a/libsepol/src/policydb.c b/libsepol/src/policydb.c > > index a6d76ca3..dc201e2f 100644 > > --- a/libsepol/src/policydb.c > > +++ b/libsepol/src/policydb.c > > @@ -2830,15 +2830,21 @@ static int ocontext_read_selinux(struct > > policydb_compat_info *info, > > break; > > case OCON_IBPKEY: > > rc = next_entry(buf, fp, sizeof(uint32_t) * > > 4); > > - if (rc < 0 || buf[2] > 0x || buf[3] > > > 0x) > > + if (rc < 0) > > return -1; > > > > + c->u.ibpkey.low_pkey = le32_to_cpu(buf[2]); > > + c->u.ibpkey.high_pkey = le32_to_cpu(buf[3]); > > Ahh you're assigning a 32 bit value to a 16 bit variable low|high_pkey. I > think > you need to assign them to a uint32_t if you want to actually range check > them. If you can, give me a chance to look over the kernel changes first. I doubt I'll see anything objectionable given the review the patches have already seen, but one never knows. > > + > > + if (c->u.ibpkey.low_pkey > 0x || > > + c->u.ibpkey.high_pkey > 0x) > > + return -1; > > + > > + /* we want c->u.ibpkey.subnet_prefix in > > network > > +* (big-endian) order, just memcpy it */ > > memcpy(>u.ibpkey.subnet_prefix, buf, > >sizeof(c->u.ibpkey.subnet_prefix)); > > > > - c->u.ibpkey.low_pkey = le32_to_cpu(buf[2]); > > - c->u.ibpkey.high_pkey = le32_to_cpu(buf[3]); > > - > > if (context_read_and_validate > > (>context[0], p, fp)) > > return -1; > > -- > > 2.17.2 > > > See job: https://travis-ci.org/SELinuxProject/selinux/jobs/442750208 > > Build fail with gcc: > > policydb.c:2839:31: error: comparison is always false due to limited > range of data type [-Werror=type-limits] > if (c->u.ibpkey.low_pkey > 0x || >^ > policydb.c:2840:31: error: comparison is always false due to limited > range of data type [-Werror=type-limits] > c->u.ibpkey.high_pkey > 0x) -- paul moore www.paul-moore.com ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH] libsepol: fix endianity in ibpkey range checks
On Wed, Oct 17, 2018 at 7:48 AM Ondrej Mosnacek wrote: > > We need to convert from little-endian before dong range checks on the > ibpkey port numbers, otherwise we would be checking a wrong value. > > Fixes: 9fbb3112769a ("libsepol: Add ibpkey ocontext handling") > Signed-off-by: Ondrej Mosnacek > --- > libsepol/src/policydb.c | 14 ++ > 1 file changed, 10 insertions(+), 4 deletions(-) > > diff --git a/libsepol/src/policydb.c b/libsepol/src/policydb.c > index a6d76ca3..dc201e2f 100644 > --- a/libsepol/src/policydb.c > +++ b/libsepol/src/policydb.c > @@ -2830,15 +2830,21 @@ static int ocontext_read_selinux(struct > policydb_compat_info *info, > break; > case OCON_IBPKEY: > rc = next_entry(buf, fp, sizeof(uint32_t) * > 4); > - if (rc < 0 || buf[2] > 0x || buf[3] > > 0x) > + if (rc < 0) > return -1; > > + c->u.ibpkey.low_pkey = le32_to_cpu(buf[2]); > + c->u.ibpkey.high_pkey = le32_to_cpu(buf[3]); Ahh you're assigning a 32 bit value to a 16 bit variable low|high_pkey. I think you need to assign them to a uint32_t if you want to actually range check them. > + > + if (c->u.ibpkey.low_pkey > 0x || > + c->u.ibpkey.high_pkey > 0x) > + return -1; > + > + /* we want c->u.ibpkey.subnet_prefix in > network > +* (big-endian) order, just memcpy it */ > memcpy(>u.ibpkey.subnet_prefix, buf, >sizeof(c->u.ibpkey.subnet_prefix)); > > - c->u.ibpkey.low_pkey = le32_to_cpu(buf[2]); > - c->u.ibpkey.high_pkey = le32_to_cpu(buf[3]); > - > if (context_read_and_validate > (>context[0], p, fp)) > return -1; > -- > 2.17.2 > See job: https://travis-ci.org/SELinuxProject/selinux/jobs/442750208 Build fail with gcc: policydb.c:2839:31: error: comparison is always false due to limited range of data type [-Werror=type-limits] if (c->u.ibpkey.low_pkey > 0x || ^ policydb.c:2840:31: error: comparison is always false due to limited range of data type [-Werror=type-limits] c->u.ibpkey.high_pkey > 0x) ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH] libsepol: fix endianity in ibpkey range checks
On Wed, Oct 17, 2018 at 8:30 AM Stephen Smalley wrote: > > On 10/17/2018 10:46 AM, Ondrej Mosnacek wrote: > > We need to convert from little-endian before dong range checks on the > > ibpkey port numbers, otherwise we would be checking a wrong value. > > > > Fixes: 9fbb3112769a ("libsepol: Add ibpkey ocontext handling") > > Signed-off-by: Ondrej Mosnacek > > Acked-by: Stephen Smalley Stephen, Ill stage this as a PR. Do you want to wait until the kernel changes are in or just the normal 24 hours? Bill > > > --- > > libsepol/src/policydb.c | 14 ++ > > 1 file changed, 10 insertions(+), 4 deletions(-) > > > > diff --git a/libsepol/src/policydb.c b/libsepol/src/policydb.c > > index a6d76ca3..dc201e2f 100644 > > --- a/libsepol/src/policydb.c > > +++ b/libsepol/src/policydb.c > > @@ -2830,15 +2830,21 @@ static int ocontext_read_selinux(struct > > policydb_compat_info *info, > > break; > > case OCON_IBPKEY: > > rc = next_entry(buf, fp, sizeof(uint32_t) * > > 4); > > - if (rc < 0 || buf[2] > 0x || buf[3] > > > 0x) > > + if (rc < 0) > > return -1; > > > > + c->u.ibpkey.low_pkey = le32_to_cpu(buf[2]); > > + c->u.ibpkey.high_pkey = le32_to_cpu(buf[3]); > > + > > + if (c->u.ibpkey.low_pkey > 0x || > > + c->u.ibpkey.high_pkey > 0x) > > + return -1; > > + > > + /* we want c->u.ibpkey.subnet_prefix in > > network > > + * (big-endian) order, just memcpy it */ > > memcpy(>u.ibpkey.subnet_prefix, buf, > > sizeof(c->u.ibpkey.subnet_prefix)); > > > - c->u.ibpkey.low_pkey = le32_to_cpu(buf[2]); > > - c->u.ibpkey.high_pkey = le32_to_cpu(buf[3]); > > - > > if (context_read_and_validate > > (>context[0], p, fp)) > > return -1; > > > > ___ > Selinux mailing list > Selinux@tycho.nsa.gov > To unsubscribe, send email to selinux-le...@tycho.nsa.gov. > To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov. ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH] libsepol: fix endianity in ibpkey range checks
On 10/17/2018 10:46 AM, Ondrej Mosnacek wrote: We need to convert from little-endian before dong range checks on the ibpkey port numbers, otherwise we would be checking a wrong value. Fixes: 9fbb3112769a ("libsepol: Add ibpkey ocontext handling") Signed-off-by: Ondrej Mosnacek Acked-by: Stephen Smalley --- libsepol/src/policydb.c | 14 ++ 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/libsepol/src/policydb.c b/libsepol/src/policydb.c index a6d76ca3..dc201e2f 100644 --- a/libsepol/src/policydb.c +++ b/libsepol/src/policydb.c @@ -2830,15 +2830,21 @@ static int ocontext_read_selinux(struct policydb_compat_info *info, break; case OCON_IBPKEY: rc = next_entry(buf, fp, sizeof(uint32_t) * 4); - if (rc < 0 || buf[2] > 0x || buf[3] > 0x) + if (rc < 0) return -1; +c->u.ibpkey.low_pkey = le32_to_cpu(buf[2]); + c->u.ibpkey.high_pkey = le32_to_cpu(buf[3]); + + if (c->u.ibpkey.low_pkey > 0x || + c->u.ibpkey.high_pkey > 0x) + return -1; + + /* we want c->u.ibpkey.subnet_prefix in network +* (big-endian) order, just memcpy it */ memcpy(>u.ibpkey.subnet_prefix, buf, sizeof(c->u.ibpkey.subnet_prefix)); > - c->u.ibpkey.low_pkey = le32_to_cpu(buf[2]); - c->u.ibpkey.high_pkey = le32_to_cpu(buf[3]); - if (context_read_and_validate (>context[0], p, fp)) return -1; ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
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); +
Re: [PATCH] libsepol: fix endianity in ibpkey range checks
On Wed, Oct 17, 2018 at 7:48 AM Ondrej Mosnacek wrote: > > We need to convert from little-endian before dong range checks on the > ibpkey port numbers, otherwise we would be checking a wrong value. > > Fixes: 9fbb3112769a ("libsepol: Add ibpkey ocontext handling") > Signed-off-by: Ondrej Mosnacek > --- > libsepol/src/policydb.c | 14 ++ > 1 file changed, 10 insertions(+), 4 deletions(-) > > diff --git a/libsepol/src/policydb.c b/libsepol/src/policydb.c > index a6d76ca3..dc201e2f 100644 > --- a/libsepol/src/policydb.c > +++ b/libsepol/src/policydb.c > @@ -2830,15 +2830,21 @@ static int ocontext_read_selinux(struct > policydb_compat_info *info, > break; > case OCON_IBPKEY: > rc = next_entry(buf, fp, sizeof(uint32_t) * > 4); > - if (rc < 0 || buf[2] > 0x || buf[3] > > 0x) > + if (rc < 0) > return -1; > > + c->u.ibpkey.low_pkey = le32_to_cpu(buf[2]); > + c->u.ibpkey.high_pkey = le32_to_cpu(buf[3]); > + > + if (c->u.ibpkey.low_pkey > 0x || > + c->u.ibpkey.high_pkey > 0x) > + return -1; > + > + /* we want c->u.ibpkey.subnet_prefix in > network > +* (big-endian) order, just memcpy it */ > memcpy(>u.ibpkey.subnet_prefix, buf, >sizeof(c->u.ibpkey.subnet_prefix)); > > - c->u.ibpkey.low_pkey = le32_to_cpu(buf[2]); > - c->u.ibpkey.high_pkey = le32_to_cpu(buf[3]); > - > if (context_read_and_validate > (>context[0], p, fp)) > return -1; > -- > 2.17.2 This seems to line up with what I have been following on the kernel side. But since Stephen committed the patch this fixes up and is way more involved, ill defer to him. Soft ack from me, but I would imagine we would want to see an ack on the kernel side before pulling this in. > > ___ > Selinux mailing list > Selinux@tycho.nsa.gov > To unsubscribe, send email to selinux-le...@tycho.nsa.gov. > To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov. ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
[PATCH] libsepol: fix endianity in ibpkey range checks
We need to convert from little-endian before dong range checks on the ibpkey port numbers, otherwise we would be checking a wrong value. Fixes: 9fbb3112769a ("libsepol: Add ibpkey ocontext handling") Signed-off-by: Ondrej Mosnacek --- libsepol/src/policydb.c | 14 ++ 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/libsepol/src/policydb.c b/libsepol/src/policydb.c index a6d76ca3..dc201e2f 100644 --- a/libsepol/src/policydb.c +++ b/libsepol/src/policydb.c @@ -2830,15 +2830,21 @@ static int ocontext_read_selinux(struct policydb_compat_info *info, break; case OCON_IBPKEY: rc = next_entry(buf, fp, sizeof(uint32_t) * 4); - if (rc < 0 || buf[2] > 0x || buf[3] > 0x) + if (rc < 0) return -1; + c->u.ibpkey.low_pkey = le32_to_cpu(buf[2]); + c->u.ibpkey.high_pkey = le32_to_cpu(buf[3]); + + if (c->u.ibpkey.low_pkey > 0x || + c->u.ibpkey.high_pkey > 0x) + return -1; + + /* we want c->u.ibpkey.subnet_prefix in network +* (big-endian) order, just memcpy it */ memcpy(>u.ibpkey.subnet_prefix, buf, sizeof(c->u.ibpkey.subnet_prefix)); - c->u.ibpkey.low_pkey = le32_to_cpu(buf[2]); - c->u.ibpkey.high_pkey = le32_to_cpu(buf[3]); - if (context_read_and_validate (>context[0], p, fp)) return -1; -- 2.17.2 ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
[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 =
Re: [PATCH v2] selinux: fix byte order and alignment issues in policydb.c
On Tue, Oct 16, 2018 at 4:19 PM Ondrej Mosnacek wrote: > On Tue, Oct 16, 2018 at 2:53 PM Stephen Smalley 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 < > > (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 > > > 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 | 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 > > > #include > > > #include > > > +#include > > > #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] > 0x || > > > - nodebuf[3] > 0x) { > > > + if (le32_to_cpu(buf[2]) > 0x || > > > + le32_to_cpu(buf[3]) > 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]); > > > + 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