| Message ID | 20231024213525.361332-7-paul@paul-moore.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | LSM syscall tweaks | expand |
On Tue, Oct 24, 2023 at 05:35:29PM -0400, Paul Moore wrote: > While we have a lsm_fill_user_ctx() helper function designed to make > life easier for LSMs which return lsm_ctx structs to userspace, we > didn't include all of the buffer length safety checks and buffer > padding adjustments in the helper. This led to code duplication > across the different LSMs and the possibility for mistakes across the > different LSM subsystems. In order to reduce code duplication and > decrease the chances of silly mistakes, we're consolidating all of > this code into the lsm_fill_user_ctx() helper. > > The buffer padding is also modified from a fixed 8-byte alignment to > an alignment that matches the word length of the machine > (BITS_PER_LONG / 8). > > Signed-off-by: Paul Moore <paul@paul-moore.com> > --- > diff --git a/security/security.c b/security/security.c > index 67ded406a5ea..45c4f5440c95 100644 > --- a/security/security.c > +++ b/security/security.c > @@ -773,42 +773,49 @@ static int lsm_superblock_alloc(struct super_block *sb) > > /** > * lsm_fill_user_ctx - Fill a user space lsm_ctx structure > - * @ctx: an LSM context to be filled > - * @context: the new context value > - * @context_size: the size of the new context value > + * @uctx: a userspace LSM context to be filled > + * @uctx_len: available uctx size (input), used uctx size (output) > + * @val: the new LSM context value > + * @val_len: the size of the new LSM context value > * @id: LSM id > * @flags: LSM defined flags > * > - * Fill all of the fields in a user space lsm_ctx structure. > - * Caller is assumed to have verified that @ctx has enough space > - * for @context. > + * Fill all of the fields in a userspace lsm_ctx structure. > * > - * Returns 0 on success, -EFAULT on a copyout error, -ENOMEM > - * if memory can't be allocated. > + * Returns 0 on success, -E2BIG if userspace buffer is not large enough, > + * -EFAULT on a copyout error, -ENOMEM if memory can't be allocated. > */ > -int lsm_fill_user_ctx(struct lsm_ctx __user *ctx, void *context, > - size_t context_size, u64 id, u64 flags) > +int lsm_fill_user_ctx(struct lsm_ctx __user *uctx, size_t *uctx_len, > + void *val, size_t val_len, > + u64 id, u64 flags) > { > - struct lsm_ctx *lctx; > - size_t locallen = struct_size(lctx, ctx, context_size); > + struct lsm_ctx *nctx = NULL; > + size_t nctx_len; > int rc = 0; > > - lctx = kzalloc(locallen, GFP_KERNEL); > - if (lctx == NULL) > - return -ENOMEM; > + nctx_len = ALIGN(struct_size(nctx, ctx, val_len), BITS_PER_LONG / 8); Why the arch-dependent constant? I'm not even sure why we want to align this size. We'll only copy the actual size right? > + if (nctx_len > *uctx_len) { > + rc = -E2BIG; > + goto out; > + } > > - lctx->id = id; > - lctx->flags = flags; > - lctx->ctx_len = context_size; > - lctx->len = locallen; > + nctx = kzalloc(nctx_len, GFP_KERNEL); > + if (nctx == NULL) { > + rc = -ENOMEM; > + goto out; > + } > + nctx->id = id; > + nctx->flags = flags; > + nctx->len = nctx_len; > + nctx->ctx_len = val_len; > + memcpy(nctx->ctx, val, val_len); > > - memcpy(lctx->ctx, context, context_size); > - > - if (copy_to_user(ctx, lctx, locallen)) > + if (copy_to_user(uctx, nctx, nctx_len)) > rc = -EFAULT; > > - kfree(lctx); > - > +out: > + kfree(nctx); > + *uctx_len = nctx_len; > return rc; > } > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > index 1fe30e635923..c32794979aab 100644 > --- a/security/selinux/hooks.c > +++ b/security/selinux/hooks.c > @@ -6480,30 +6480,32 @@ static int selinux_lsm_setattr(u64 attr, void *value, size_t size) > return error; > } > > +/** > + * selinux_getselfattr - Get SELinux current task attributes > + * @attr: the requested attribute > + * @ctx: buffer to receive the result > + * @size: buffer size (input), buffer size used (output) > + * @flags: unused > + * > + * Fill the passed user space @ctx with the details of the requested > + * attribute. > + * > + * Returns the number of attributes on success, an error code otherwise. > + * There will only ever be one attribute. > + */ > static int selinux_getselfattr(unsigned int attr, struct lsm_ctx __user *ctx, > size_t *size, u32 flags) > { > - char *value; > - size_t total_len; > - int len; > - int rc = 0; > + int rc; > + char *val; > + int val_len; > > - len = selinux_lsm_getattr(attr, current, &value); > - if (len < 0) > - return len; > - > - total_len = ALIGN(struct_size(ctx, ctx, len), 8); > - > - if (total_len > *size) > - rc = -E2BIG; > - else if (ctx) > - rc = lsm_fill_user_ctx(ctx, value, len, LSM_ID_SELINUX, 0); > - > - kfree(value); > - *size = total_len; > - if (rc < 0) > - return rc; > - return 1; > + val_len = selinux_lsm_getattr(attr, current, &val); > + if (val_len < 0) > + return val_len; > + rc = lsm_fill_user_ctx(ctx, size, val, val_len, LSM_ID_SELINUX, 0); > + kfree(val); > + return (!rc ? 1 : rc); > } > > static int selinux_setselfattr(unsigned int attr, struct lsm_ctx *ctx,
On Thu, Oct 26, 2023 at 11:13 AM Mickaël Salaün <mic@digikod.net> wrote: > On Tue, Oct 24, 2023 at 05:35:29PM -0400, Paul Moore wrote: > > While we have a lsm_fill_user_ctx() helper function designed to make > > life easier for LSMs which return lsm_ctx structs to userspace, we > > didn't include all of the buffer length safety checks and buffer > > padding adjustments in the helper. This led to code duplication > > across the different LSMs and the possibility for mistakes across the > > different LSM subsystems. In order to reduce code duplication and > > decrease the chances of silly mistakes, we're consolidating all of > > this code into the lsm_fill_user_ctx() helper. > > > > The buffer padding is also modified from a fixed 8-byte alignment to > > an alignment that matches the word length of the machine > > (BITS_PER_LONG / 8). > > > > Signed-off-by: Paul Moore <paul@paul-moore.com> > > --- > > > diff --git a/security/security.c b/security/security.c > > index 67ded406a5ea..45c4f5440c95 100644 > > --- a/security/security.c > > +++ b/security/security.c ... > > +int lsm_fill_user_ctx(struct lsm_ctx __user *uctx, size_t *uctx_len, > > + void *val, size_t val_len, > > + u64 id, u64 flags) > > { > > - struct lsm_ctx *lctx; > > - size_t locallen = struct_size(lctx, ctx, context_size); > > + struct lsm_ctx *nctx = NULL; > > + size_t nctx_len; > > int rc = 0; > > > > - lctx = kzalloc(locallen, GFP_KERNEL); > > - if (lctx == NULL) > > - return -ENOMEM; > > + nctx_len = ALIGN(struct_size(nctx, ctx, val_len), BITS_PER_LONG / 8); > > Why the arch-dependent constant? My thinking is that most arches tend to perform better when data is aligned on a word boundary and this should help achieve that in a way that doesn't assume the arch's word length. If you have an idea on how to do this differently I'm open to suggestions. It's worth noting that this is something we can change in the future as the lsm_ctx struct has the len field which we can use for arbitrary amounts of padding, including none. > I'm not even sure why we want to align this size. We'll only copy the > actual size right? We allocate, zero out, and copy @nctx_len/@nctx->len. > > + if (nctx_len > *uctx_len) { > > + rc = -E2BIG; > > + goto out; > > + } > > > > - lctx->id = id; > > - lctx->flags = flags; > > - lctx->ctx_len = context_size; > > - lctx->len = locallen; > > + nctx = kzalloc(nctx_len, GFP_KERNEL); > > + if (nctx == NULL) { > > + rc = -ENOMEM; > > + goto out; > > + } > > + nctx->id = id; > > + nctx->flags = flags; > > + nctx->len = nctx_len; > > + nctx->ctx_len = val_len; > > + memcpy(nctx->ctx, val, val_len); > > > > - memcpy(lctx->ctx, context, context_size); > > - > > - if (copy_to_user(ctx, lctx, locallen)) > > + if (copy_to_user(uctx, nctx, nctx_len)) > > rc = -EFAULT; > > > > - kfree(lctx); > > - > > +out: > > + kfree(nctx); > > + *uctx_len = nctx_len; > > return rc; > > }
On 24/10/2023 22:35, Paul Moore wrote: > While we have a lsm_fill_user_ctx() helper function designed to make > life easier for LSMs which return lsm_ctx structs to userspace, we > didn't include all of the buffer length safety checks and buffer > padding adjustments in the helper. This led to code duplication > across the different LSMs and the possibility for mistakes across the > different LSM subsystems. In order to reduce code duplication and > decrease the chances of silly mistakes, we're consolidating all of > this code into the lsm_fill_user_ctx() helper. > > The buffer padding is also modified from a fixed 8-byte alignment to > an alignment that matches the word length of the machine > (BITS_PER_LONG / 8). > > Signed-off-by: Paul Moore <paul@paul-moore.com> > --- > include/linux/security.h | 9 ++++--- > security/apparmor/lsm.c | 15 +++-------- > security/security.c | 55 +++++++++++++++++++++----------------- > security/selinux/hooks.c | 42 +++++++++++++++-------------- > security/smack/smack_lsm.c | 23 +++++----------- > 5 files changed, 67 insertions(+), 77 deletions(-) > Hi Paul, While building the kernel against next-master for arch arm64 > security/security.c:810:2: warning: ‘memcpy’ offset 32 is out of the bounds [0, 0] [-Warray-bounds] warning is observed. On some other architectures like i386 and x86_64, an error is observed. > arch/x86/include/asm/string_32.h:150:25: error: ‘__builtin_memcpy’ offset 32 is out of the bounds [0, 0] [-Werror=array-bounds] The links of the logs is listed below: https://storage.kernelci.org/next/master/next-20231220/arm64/defconfig/gcc-10/logs/build-warnings.log https://storage.kernelci.org/next/master/next-20231220/i386/i386_defconfig/gcc-10/logs/build-errors.log The logs of all the architecture built against next-master can be found here (select the 'All' category in the table to view): https://linux.kernelci.org/build/next/branch/master/kernel/next-20231220/ Find this issue filed at KSPP/linux here: https://github.com/KSPP/linux/issues/347 A bisect done by building kernel against next-master for arch arm64 (full log below) identified this patch as introducing the failure. git bisect log: git bisect start # good: [b85ea95d086471afb4ad062012a4d73cd328fa86] Linux 6.7-rc1 git bisect good b85ea95d086471afb4ad062012a4d73cd328fa86 # bad: [5ba73bec5e7b0494da7fdca3e003d8b97fa932cd] Add linux-next specific files for 20231114 git bisect bad 5ba73bec5e7b0494da7fdca3e003d8b97fa932cd # good: [a15c6466b909f03889150df57b227702a7bd6bd5] Merge branch 'for-next' of git://git.kernel.org/pub/scm/linux/kernel/git/wireless/wireless-next.git git bisect good a15c6466b909f03889150df57b227702a7bd6bd5 # good: [6a8b8b208098a27488a3649966d64894da948a02] Merge branch 'for-next' of git://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git git bisect good 6a8b8b208098a27488a3649966d64894da948a02 # bad: [81105901f053f9684a111c0569eb35474b2a86f9] Merge branch 'next' of git://git.kernel.org/pub/scm/linux/kernel/git/coresight/linux.git git bisect bad 81105901f053f9684a111c0569eb35474b2a86f9 # bad: [585a8722efb6f823e961f16bd9be818f994d4804] Merge branch 'rcu/next' of git://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git git bisect bad 585a8722efb6f823e961f16bd9be818f994d4804 # good: [c867caae623b3dd488a849df5538e79a59b0a47f] Merge branch into tip/master: 'x86/percpu' git bisect good c867caae623b3dd488a849df5538e79a59b0a47f # bad: [381a25d3e3d440ccc05de8ddd56a055423ac9fe5] Merge branch 'next' of git://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/lsm.git git bisect bad 381a25d3e3d440ccc05de8ddd56a055423ac9fe5 # good: [762c934317e6f4b576eb4aa75e5facf4968a4a8f] SELinux: Add selfattr hooks git bisect good 762c934317e6f4b576eb4aa75e5facf4968a4a8f # good: [fdcf699b60712ecd6e41d9fc09137279257a4bf8] lsm: correct error codes in security_getselfattr() git bisect good fdcf699b60712ecd6e41d9fc09137279257a4bf8 # bad: [9ba8802c8b66fbde2ee32ab4c44cd418f9444486] lsm: convert security_setselfattr() to use memdup_user() git bisect bad 9ba8802c8b66fbde2ee32ab4c44cd418f9444486 # bad: [41793202292fd2acf99fdc09eff8323cc27c80eb] lsm: align based on pointer length in lsm_fill_user_ctx() git bisect bad 41793202292fd2acf99fdc09eff8323cc27c80eb # bad: [d7cf3412a9f6c547e5ee443fa7644e08898aa3e2] lsm: consolidate buffer size handling into lsm_fill_user_ctx() git bisect bad d7cf3412a9f6c547e5ee443fa7644e08898aa3e2 # first bad commit: [d7cf3412a9f6c547e5ee443fa7644e08898aa3e2] lsm: consolidate buffer size handling into lsm_fill_user_ctx() Thanks, Aishwarya
On Wed, Dec 20, 2023 at 5:31 PM Aishwarya TCV <aishwarya.tcv@arm.com> wrote: > On 24/10/2023 22:35, Paul Moore wrote: > > While we have a lsm_fill_user_ctx() helper function designed to make > > life easier for LSMs which return lsm_ctx structs to userspace, we > > didn't include all of the buffer length safety checks and buffer > > padding adjustments in the helper. This led to code duplication > > across the different LSMs and the possibility for mistakes across the > > different LSM subsystems. In order to reduce code duplication and > > decrease the chances of silly mistakes, we're consolidating all of > > this code into the lsm_fill_user_ctx() helper. > > > > The buffer padding is also modified from a fixed 8-byte alignment to > > an alignment that matches the word length of the machine > > (BITS_PER_LONG / 8). > > > > Signed-off-by: Paul Moore <paul@paul-moore.com> > > --- > > include/linux/security.h | 9 ++++--- > > security/apparmor/lsm.c | 15 +++-------- > > security/security.c | 55 +++++++++++++++++++++----------------- > > security/selinux/hooks.c | 42 +++++++++++++++-------------- > > security/smack/smack_lsm.c | 23 +++++----------- > > 5 files changed, 67 insertions(+), 77 deletions(-) > > Hi Paul, > > While building the kernel against next-master for arch arm64 > > security/security.c:810:2: warning: ‘memcpy’ offset 32 is out of the bounds [0, 0] [-Warray-bounds] > warning is observed. On some other architectures like i386 and x86_64, > an error is observed. > arch/x86/include/asm/string_32.h:150:25: error: > ‘__builtin_memcpy’ offset 32 is out of the bounds [0, 0] > [-Werror=array-bounds] I believe the code is correct, I'm guessing this is simply a question of the compiler not seeing whatever syntactic magic is required for your compilation flags. While I'm not entirely sure of the "[0, 0]" "bounds" in the warning/error message, if that were a offset/limit/length a double zero value would also seem to indicate this is more of a compiler annotation issue than a code issue. Looking at the lsm_ctx definition in include/uapi/linux/lsm.h I see the following: struct lsm_ctx { __u64 id; /* offset: 0 */ __u64 flags; /* offset: 8 */ __u64 len; /* offset: 16 */ __u64 ctx_len; /* offset: 24 */ __u8 ctx[]; /* offset: 32 */ }; and given that the offending line of code is trying to do a memcpy into the ctx field, an offset of 32 looks correct to me. Suggestions on how to annotate the struct, or the code doing the memcpy() are welcome.
On Wed, Dec 20, 2023 at 08:40:24PM -0500, Paul Moore wrote: > Looking at the lsm_ctx definition in include/uapi/linux/lsm.h I see > the following: > struct lsm_ctx { > __u64 id; /* offset: 0 */ > __u64 flags; /* offset: 8 */ > __u64 len; /* offset: 16 */ > __u64 ctx_len; /* offset: 24 */ > __u8 ctx[]; /* offset: 32 */ > }; > and given that the offending line of code is trying to do a memcpy > into the ctx field, an offset of 32 looks correct to me. > Suggestions on how to annotate the struct, or the code doing the > memcpy() are welcome. You're looking for a __counted_by() annotation here I think.
On Thu, Dec 21, 2023 at 8:01 AM Mark Brown <broonie@kernel.org> wrote: > On Wed, Dec 20, 2023 at 08:40:24PM -0500, Paul Moore wrote: > > Looking at the lsm_ctx definition in include/uapi/linux/lsm.h I see > > the following: > > > struct lsm_ctx { > > __u64 id; /* offset: 0 */ > > __u64 flags; /* offset: 8 */ > > __u64 len; /* offset: 16 */ > > __u64 ctx_len; /* offset: 24 */ > > __u8 ctx[]; /* offset: 32 */ > > }; > > > and given that the offending line of code is trying to do a memcpy > > into the ctx field, an offset of 32 looks correct to me. > > > Suggestions on how to annotate the struct, or the code doing the > > memcpy() are welcome. > > You're looking for a __counted_by() annotation here I think. Can you verify and submit a patch for that? I'm asking because my build/toolchain configuration never produced these warnings/errors during my testing.
On Thu, Dec 21, 2023 at 10:21:04AM -0500, Paul Moore wrote: > On Thu, Dec 21, 2023 at 8:01 AM Mark Brown <broonie@kernel.org> wrote: > > On Wed, Dec 20, 2023 at 08:40:24PM -0500, Paul Moore wrote: > > > Suggestions on how to annotate the struct, or the code doing the > > > memcpy() are welcome. > > You're looking for a __counted_by() annotation here I think. > Can you verify and submit a patch for that? I'm asking because my > build/toolchain configuration never produced these warnings/errors > during my testing. Huh, actually it's not __counted_by() since this shows up with arm64/gcc-10 (and some other arches) which doesn't have that. I'll submit the __counted_by() patch anyway since it's clearly a good annotation but it doesn't actually shut the warning up in this case. Adding the hardening people, and I'll have a further look. You can reproduce with tuxmake -r docker -k defconfig -a arm64 -t gcc-10 using https://www.tuxmake.org/
diff --git a/include/linux/security.h b/include/linux/security.h index 334f75aa7289..750130a7b9dd 100644 --- a/include/linux/security.h +++ b/include/linux/security.h @@ -492,8 +492,8 @@ int security_inode_notifysecctx(struct inode *inode, void *ctx, u32 ctxlen); int security_inode_setsecctx(struct dentry *dentry, void *ctx, u32 ctxlen); int security_inode_getsecctx(struct inode *inode, void **ctx, u32 *ctxlen); int security_locked_down(enum lockdown_reason what); -int lsm_fill_user_ctx(struct lsm_ctx __user *ctx, void *context, - size_t context_size, u64 id, u64 flags); +int lsm_fill_user_ctx(struct lsm_ctx __user *uctx, size_t *uctx_len, + void *val, size_t val_len, u64 id, u64 flags); #else /* CONFIG_SECURITY */ static inline int call_blocking_lsm_notifier(enum lsm_event event, void *data) @@ -1424,8 +1424,9 @@ static inline int security_locked_down(enum lockdown_reason what) { return 0; } -static inline int lsm_fill_user_ctx(struct lsm_ctx __user *ctx, void *context, - size_t context_size, u64 id, u64 flags) +static inline int lsm_fill_user_ctx(struct lsm_ctx __user *uctx, + size_t *uctx_len, void *val, size_t val_len, + u64 id, u64 flags) { return -EOPNOTSUPP; } diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c index 5e16c03936b9..6df97eb6e7d9 100644 --- a/security/apparmor/lsm.c +++ b/security/apparmor/lsm.c @@ -636,7 +636,6 @@ static int apparmor_getselfattr(unsigned int attr, struct lsm_ctx __user *lx, int error = -ENOENT; struct aa_task_ctx *ctx = task_ctx(current); struct aa_label *label = NULL; - size_t total_len = 0; char *value; switch (attr) { @@ -658,22 +657,14 @@ static int apparmor_getselfattr(unsigned int attr, struct lsm_ctx __user *lx, if (label) { error = aa_getprocattr(label, &value, false); - if (error > 0) { - total_len = ALIGN(struct_size(lx, ctx, error), 8); - if (total_len > *size) - error = -E2BIG; - else if (lx) - error = lsm_fill_user_ctx(lx, value, error, - LSM_ID_APPARMOR, 0); - else - error = 1; - } + if (error > 0) + error = lsm_fill_user_ctx(lx, size, value, error, + LSM_ID_APPARMOR, 0); kfree(value); } aa_put_label(label); - *size = total_len; if (error < 0) return error; return 1; diff --git a/security/security.c b/security/security.c index 67ded406a5ea..45c4f5440c95 100644 --- a/security/security.c +++ b/security/security.c @@ -773,42 +773,49 @@ static int lsm_superblock_alloc(struct super_block *sb) /** * lsm_fill_user_ctx - Fill a user space lsm_ctx structure - * @ctx: an LSM context to be filled - * @context: the new context value - * @context_size: the size of the new context value + * @uctx: a userspace LSM context to be filled + * @uctx_len: available uctx size (input), used uctx size (output) + * @val: the new LSM context value + * @val_len: the size of the new LSM context value * @id: LSM id * @flags: LSM defined flags * - * Fill all of the fields in a user space lsm_ctx structure. - * Caller is assumed to have verified that @ctx has enough space - * for @context. + * Fill all of the fields in a userspace lsm_ctx structure. * - * Returns 0 on success, -EFAULT on a copyout error, -ENOMEM - * if memory can't be allocated. + * Returns 0 on success, -E2BIG if userspace buffer is not large enough, + * -EFAULT on a copyout error, -ENOMEM if memory can't be allocated. */ -int lsm_fill_user_ctx(struct lsm_ctx __user *ctx, void *context, - size_t context_size, u64 id, u64 flags) +int lsm_fill_user_ctx(struct lsm_ctx __user *uctx, size_t *uctx_len, + void *val, size_t val_len, + u64 id, u64 flags) { - struct lsm_ctx *lctx; - size_t locallen = struct_size(lctx, ctx, context_size); + struct lsm_ctx *nctx = NULL; + size_t nctx_len; int rc = 0; - lctx = kzalloc(locallen, GFP_KERNEL); - if (lctx == NULL) - return -ENOMEM; + nctx_len = ALIGN(struct_size(nctx, ctx, val_len), BITS_PER_LONG / 8); + if (nctx_len > *uctx_len) { + rc = -E2BIG; + goto out; + } - lctx->id = id; - lctx->flags = flags; - lctx->ctx_len = context_size; - lctx->len = locallen; + nctx = kzalloc(nctx_len, GFP_KERNEL); + if (nctx == NULL) { + rc = -ENOMEM; + goto out; + } + nctx->id = id; + nctx->flags = flags; + nctx->len = nctx_len; + nctx->ctx_len = val_len; + memcpy(nctx->ctx, val, val_len); - memcpy(lctx->ctx, context, context_size); - - if (copy_to_user(ctx, lctx, locallen)) + if (copy_to_user(uctx, nctx, nctx_len)) rc = -EFAULT; - kfree(lctx); - +out: + kfree(nctx); + *uctx_len = nctx_len; return rc; } diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index 1fe30e635923..c32794979aab 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -6480,30 +6480,32 @@ static int selinux_lsm_setattr(u64 attr, void *value, size_t size) return error; } +/** + * selinux_getselfattr - Get SELinux current task attributes + * @attr: the requested attribute + * @ctx: buffer to receive the result + * @size: buffer size (input), buffer size used (output) + * @flags: unused + * + * Fill the passed user space @ctx with the details of the requested + * attribute. + * + * Returns the number of attributes on success, an error code otherwise. + * There will only ever be one attribute. + */ static int selinux_getselfattr(unsigned int attr, struct lsm_ctx __user *ctx, size_t *size, u32 flags) { - char *value; - size_t total_len; - int len; - int rc = 0; + int rc; + char *val; + int val_len; - len = selinux_lsm_getattr(attr, current, &value); - if (len < 0) - return len; - - total_len = ALIGN(struct_size(ctx, ctx, len), 8); - - if (total_len > *size) - rc = -E2BIG; - else if (ctx) - rc = lsm_fill_user_ctx(ctx, value, len, LSM_ID_SELINUX, 0); - - kfree(value); - *size = total_len; - if (rc < 0) - return rc; - return 1; + val_len = selinux_lsm_getattr(attr, current, &val); + if (val_len < 0) + return val_len; + rc = lsm_fill_user_ctx(ctx, size, val, val_len, LSM_ID_SELINUX, 0); + kfree(val); + return (!rc ? 1 : rc); } static int selinux_setselfattr(unsigned int attr, struct lsm_ctx *ctx, diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c index 12160d060cc1..99664c8cf867 100644 --- a/security/smack/smack_lsm.c +++ b/security/smack/smack_lsm.c @@ -3642,28 +3642,17 @@ static void smack_d_instantiate(struct dentry *opt_dentry, struct inode *inode) static int smack_getselfattr(unsigned int attr, struct lsm_ctx __user *ctx, size_t *size, u32 flags) { - struct smack_known *skp = smk_of_current(); - int total; - int slen; int rc; + struct smack_known *skp; if (attr != LSM_ATTR_CURRENT) return -EOPNOTSUPP; - slen = strlen(skp->smk_known) + 1; - total = ALIGN(slen + sizeof(*ctx), 8); - if (total > *size) - rc = -E2BIG; - else if (ctx) - rc = lsm_fill_user_ctx(ctx, skp->smk_known, slen, LSM_ID_SMACK, - 0); - else - rc = 1; - - *size = total; - if (rc >= 0) - return 1; - return rc; + skp = smk_of_current(); + rc = lsm_fill_user_ctx(ctx, size, + skp->smk_known, strlen(skp->smk_known) + 1, + LSM_ID_SMACK, 0); + return (!rc ? 1 : rc); } /**
While we have a lsm_fill_user_ctx() helper function designed to make life easier for LSMs which return lsm_ctx structs to userspace, we didn't include all of the buffer length safety checks and buffer padding adjustments in the helper. This led to code duplication across the different LSMs and the possibility for mistakes across the different LSM subsystems. In order to reduce code duplication and decrease the chances of silly mistakes, we're consolidating all of this code into the lsm_fill_user_ctx() helper. The buffer padding is also modified from a fixed 8-byte alignment to an alignment that matches the word length of the machine (BITS_PER_LONG / 8). Signed-off-by: Paul Moore <paul@paul-moore.com> --- include/linux/security.h | 9 ++++--- security/apparmor/lsm.c | 15 +++-------- security/security.c | 55 +++++++++++++++++++++----------------- security/selinux/hooks.c | 42 +++++++++++++++-------------- security/smack/smack_lsm.c | 23 +++++----------- 5 files changed, 67 insertions(+), 77 deletions(-)