Re: [RFC PATCH 3/3] lsm: consolidate buffer size handling into lsm_fill_user_ctx()

From: Paul Moore <paul@paul-moore.com>
To: Aishwarya TCV <aishwarya.tcv@arm.com>
Cc: linux-security-module@vger.kernel.org, selinux@vger.kernel.org,
	"Casey Schaufler" <casey@schaufler-ca.com>,
	"John Johansen" <john.johansen@canonical.com>,
	"Mickaël Salaün" <mic@digikod.net>,
	"Mark Brown" <broonie@kernel.org>
Subject: Re: [RFC PATCH 3/3] lsm: consolidate buffer size handling into lsm_fill_user_ctx()
Date: Wed, 20 Dec 2023 20:40:24 -0500	[thread overview]
Message-ID: <CAHC9VhSABo8PO0q0EUSw7uh+r8CjPFTwK6DJk8Vs4oDcckPwHg@mail.gmail.com> (raw)
In-Reply-To: <3717b995-5209-4db8-be77-c6303bb1c0db@arm.com>

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.

-- 
paul-moore.com