linux-kernel - Re: [PATCH v2 4/6] LSM: lsm_context in security_dentry_init

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <6dc1c686-108e-41a2-bda3-8744e5f7626f@schaufler-ca.com>
Date: Tue, 22 Oct 2024 09:46:57 -0700
From: Casey Schaufler <casey@...aufler-ca.com>
To: Paul Moore <paul@...l-moore.com>
Cc: linux-security-module@...r.kernel.org, jmorris@...ei.org,
 serge@...lyn.com, keescook@...omium.org, john.johansen@...onical.com,
 penguin-kernel@...ove.sakura.ne.jp, stephen.smalley.work@...il.com,
 linux-kernel@...r.kernel.org, selinux@...r.kernel.org, mic@...ikod.net,
 ceph-devel@...r.kernel.org, linux-nfs@...r.kernel.org,
 Casey Schaufler <casey@...aufler-ca.com>
Subject: Re: [PATCH v2 4/6] LSM: lsm_context in security_dentry_init_security

On 10/22/2024 9:35 AM, Paul Moore wrote:
> On Mon, Oct 21, 2024 at 8:00 PM Casey Schaufler <casey@...aufler-ca.com> wrote:
>> On 10/21/2024 4:39 PM, Paul Moore wrote:
>>> On Oct 14, 2024 Casey Schaufler <casey@...aufler-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@...aufler-ca.com>
>>>> Cc: ceph-devel@...r.kernel.org
>>>> Cc: linux-nfs@...r.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.

This came down to a choice about where the ugly code would be.
I'll restore the old behavior.

>
> 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.
>