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@xxxxxxxxxxxxxx>
> ---
> 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,
