linux-kernel - Re: [PATCH] lsm: simplify security_inode_copy_up

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <9dc21c28-1a4b-4d88-a67c-7b5050cdf0b8@schaufler-ca.com>
Date: Thu, 31 Jul 2025 08:00:54 -0700
From: Casey Schaufler <casey@...aufler-ca.com>
To: "tianjia.zhang" <tianjia.zhang@...ux.alibaba.com>,
 Paul Moore <paul@...l-moore.com>
Cc: James Morris <jmorris@...ei.org>, "Serge E. Hallyn" <serge@...lyn.com>,
 linux-security-module@...r.kernel.org, linux-kernel@...r.kernel.org,
 Casey Schaufler <casey@...aufler-ca.com>
Subject: Re: [PATCH] lsm: simplify security_inode_copy_up_xattr()

On 7/31/2025 4:59 AM, tianjia.zhang wrote:
>
>
> On 7/29/25 11:09 PM, Paul Moore wrote:
>> On Tue, Jul 29, 2025 at 10:43 AM Casey Schaufler
>> <casey@...aufler-ca.com> wrote:
>>> On 7/29/2025 2:09 AM, Tianjia Zhang wrote:
>>>> The implementation of function security_inode_copy_up_xattr can be
>>>> simplified to directly call call_int_hook().
>>>>
>>>> Signed-off-by: Tianjia Zhang <tianjia.zhang@...ux.alibaba.com>
>>>> ---
>>>>   security/security.c | 8 +-------
>>>>   1 file changed, 1 insertion(+), 7 deletions(-)
>>>>
>>>> diff --git a/security/security.c b/security/security.c
>>>> index 596d41818577..a5c2e5a8009f 100644
>>>> --- a/security/security.c
>>>> +++ b/security/security.c
>>>> @@ -2774,13 +2774,7 @@ EXPORT_SYMBOL(security_inode_copy_up);
>>>>    */
>>>>   int security_inode_copy_up_xattr(struct dentry *src, const char
>>>> *name)
>>>>   {
>>>> -     int rc;
>>>> -
>>>> -     rc = call_int_hook(inode_copy_up_xattr, src, name);
>>>> -     if (rc != LSM_RET_DEFAULT(inode_copy_up_xattr))
>>>> -             return rc;
>>>> -
>>>> -     return LSM_RET_DEFAULT(inode_copy_up_xattr);
>>>> +     return call_int_hook(inode_copy_up_xattr, src, name);
>>>
>>> Both the existing code and the proposed change are incorrect.
>>> If two LSMs supply the hook, and the first does not recognize
>>> the attribute, the second, which might recognize the attribute,
>>> will not be called. As SELinux and EVM both supply this hook
>>> there may be a real problem here.
>>
>> It appears that Smack also supplies a inode_copy_up_xattr() callback
>> via smack_inode_copy_up_xattr().
>>
>> Someone should double check this logic, but looking at it very
>> quickly, it would appear that LSM framework should run the individual
>> LSM callbacks in order so long as they return -EOPNOTSUPP, if they do
>> not return -EOPNOTSUPP, the return value should be returned to the
>> caller without executing any further callbacks.  As a default return
>> value, or if all of the LSM callbacks succeed with -EOPNOTSUPP, the
>> hook should return -EOPNOTSUPP.
>>
>> Tianjia Zhang, would you be able to develop and test a patch for this?
>>
>
> I carefully checked the logic of security_inode_copy_up_xattr(). I think
> there is no problem with the current code. The default return value of
> inode_copy_up_xattr LSM is -EOPNOTSUPP. Therefore, when -EOPNOTSUPP is
> returned in the LSM callback, the next callback function will be called
> in a loop. When an LSM module recognizes the attribute name that needs
> to be ignored, it will return -ECANCELED to indicate
> security_inode_copy_up_xattr() to jump out of the loop and ignore the
> copy of this attribute in overlayfs.
>
> Currently, the SELinux, EVM, and Smack that supply inode_copy_up_xattr
> callback all return -ECANCELED after recognizing the extended attribute
> name they are concerned about, to indicate overlayfs to discard the
> copy_up operation of this attribute. I think this is in line with
> expectations.

I looked at the code more carefully and I think you're right.
My objection was based on the behavior of a much earlier version
of call_int_hook(). With that, I think your proposed change is
reasonable.

>
> Tianjia,
> Cheers