[PATCH v2 4/6] LSM: lsm_context in security_dentry_init_security

[Paul Moore]

On Mon, Oct 21, 2024 at 8:00 PM Casey Schaufler <casey at schaufler-ca.com> wrote:
> On 10/21/2024 4:39 PM, Paul Moore wrote:
> > On Oct 14, 2024 Casey Schaufler <casey at schaufler-ca.com> wrote:
> >> Replace the (secctx,seclen) pointer pair with a single lsm_context
> >> pointer to allow return of the LSM identifier along with the context
> >> and context length. This allows security_release_secctx() to know how
> >> to release the context. Callers have been modified to use or save the
> >> returned data from the new structure.
> >>
> >> Special care is taken in the NFS code, which uses the same data structure
> >> for its own copied labels as it does for the data which comes from
> >> security_dentry_init_security().  In the case of copied labels the data
> >> has to be freed, not released.
> >>
> >> The scaffolding funtion lsmcontext_init() is no longer needed and is
> >> removed.
> >>
> >> Signed-off-by: Casey Schaufler <casey at schaufler-ca.com>
> >> Cc: ceph-devel at vger.kernel.org
> >> Cc: linux-nfs at vger.kernel.org
> >> ---
> >>  fs/ceph/super.h               |  3 +--
> >>  fs/ceph/xattr.c               | 16 ++++++----------
> >>  fs/fuse/dir.c                 | 35 ++++++++++++++++++-----------------
> >>  fs/nfs/dir.c                  |  2 +-
> >>  fs/nfs/inode.c                | 17 ++++++++++-------
> >>  fs/nfs/internal.h             |  8 +++++---
> >>  fs/nfs/nfs4proc.c             | 22 +++++++++-------------
> >>  fs/nfs/nfs4xdr.c              | 22 ++++++++++++----------
> >>  include/linux/lsm_hook_defs.h |  2 +-
> >>  include/linux/nfs4.h          |  8 ++++----
> >>  include/linux/nfs_fs.h        |  2 +-
> >>  include/linux/security.h      | 26 +++-----------------------
> >>  security/security.c           |  9 ++++-----
> >>  security/selinux/hooks.c      |  9 +++++----
> >>  14 files changed, 80 insertions(+), 101 deletions(-)

...

> >> diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h
> >> index 039898d70954..47652d217d05 100644
> >> --- a/include/linux/nfs_fs.h
> >> +++ b/include/linux/nfs_fs.h
> >> @@ -457,7 +457,7 @@ static inline void nfs4_label_free(struct nfs4_label *label)
> >>  {
> >>  #ifdef CONFIG_NFS_V4_SECURITY_LABEL
> >>      if (label) {
> >> -            kfree(label->label);
> >> +            kfree(label->lsmctx.context);
> > Shouldn't this be a call to security_release_secctx() instead of a raw
> > kfree()?
>
> As mentioned in the description, the NFS data is a copy that NFS
> manages, so it does need to be freed, not released.

It does, my apologies.

However, this makes me wonder if using the lsm_context struct for the
private NFS copy is the right decision.  The NFS code assumes and
requires a single string, ala secctx, but I think we want the ability
to potentially do other/additional things with lsm_context, even if
this patchset doesn't do that.

I would suggest keeping the NFS private copy as sec_ctx/sec_ctxlen and
keep the concept of a translation between the data structures in
place, even though it is just a simple string duplication right now.

-- 
paul-moore.com