| 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
| ||
|
Date: Fri, 21 Jun 2024 09:42:20 +0530
From: Neeraj Upadhyay <Neeraj.Upadhyay@....com>
To: John Johansen <john.johansen@...onical.com>,
Mateusz Guzik <mjguzik@...il.com>
Cc: paul@...l-moore.com, jmorris@...ei.org, serge@...lyn.com,
linux-kernel@...r.kernel.org, apparmor@...ts.ubuntu.com,
linux-security-module@...r.kernel.org
Subject: Re: [PATCH v2] apparmor: try to avoid refing the label in
apparmor_file_open
On 6/21/2024 12:33 AM, John Johansen wrote:
> On 6/20/24 11:30, Mateusz Guzik wrote:
>> On Thu, Jun 20, 2024 at 8:23 PM Neeraj Upadhyay <Neeraj.Upadhyay@....com> wrote:
>>>
>>>
>>>
>>> On 6/20/2024 10:45 PM, Mateusz Guzik wrote:
>>>> apparmor: try to avoid refing the label in apparmor_file_open
>>>>
>>>> If the label is not stale (which is the common case), the fact that the
>>>> passed file object holds a reference can be leverged to avoid the
>>>
>>> Minor: Typo 'leveraged'
>>>
>>>> ref/unref cycle. Doing so reduces performance impact of apparmor on
>>>> parallel open() invocations.
>>>>
>>>> When benchmarking on a 24-core vm using will-it-scale's open1_process
>>>> ("Separate file open"), the results are (ops/s):
>>>> before: 6092196
>>>> after: 8309726 (+36%)
>>>>
>>>> Signed-off-by: Mateusz Guzik <mjguzik@...il.com>
>>>> ---
>>>
>>>
>>> Trying to understand the changes done here. So, while the file cred can be updated
>>> async to the task (referring to the comment from John here [1]), the file cred label
>>> cannot change during apparmor_file_open() execution?
>>>
>>
>> Refing a label retains racy vs refing it.
>>
>> On stock code you can test the flag, determine it's not stale, grab
>> the ref and have it become stale immediately after. My patch avoids
>> the atomic dance for the common case, does not alter anything
>> correctness-wise AFAICS.
>>
>> I am assuming the race is tolerated and checking here is only done to
>> make sure the new label is seen eventually.
>>
>> Not having the race is possible with a bunch of trickery like seqc,
>> but so far does not look like this is necessary.
>>
>
> the race is some what tolerated because of the nature of what is being
> done with the label. Basically labels go stale with policy updates, and
> we do not guarantee atomic policy updates as the locking to do so would
> cause a lot of performance issues.
>
> Generally for mediation, the label is used in a read only fashion and
> the state is taken at the start of hook entry and used the read side
> value for the duration of the hook.
>
> If the hook is to update the label it must take a lock and then get
> any updated state before continuing to update the label.
>
> This case in particular is unique in that apparmor doesn't update
> the f_cred. The label can go stale but the file will continue to
> reference the original label frpm the time the f_cred was set. So there
> is no race that the f_creds reference might be put. This does however
> mean that policy updates might result in the slow path, having to do
> a refcount, getting triggered.
>
> The afore mentioned replacement of unconfined and object delegation
> work will change what apparmor is doing here and break this but like
> I said that is a problem for those patches in the future.
>
>
Thank you both for sharing the details!
Thanks
Neeraj
>>>
>>> Reviewed-by: Neeraj upadhyay <Neeraj.Upadhyay@....com>
>>>
>>>
>>> Thanks
>>> Neeraj
>>>
>>> [1] https://lore.kernel.org/lkml/9bfaeec2-535d-4401-8244-7560f660a065@canonical.com/
>>>
>>>
>>>>
>>>> v2:
>>>> - reword the commit message
>>>>
>>>> If you want any changes made to it can you just do them on your own
>>>> accord? :) Will be faster for both of us than another mail trip.
>>>>
>>>> security/apparmor/include/cred.h | 20 ++++++++++++++++++++
>>>> security/apparmor/lsm.c | 5 +++--
>>>> 2 files changed, 23 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/security/apparmor/include/cred.h b/security/apparmor/include/cred.h
>>>> index 58fdc72af664..7265d2f81dd5 100644
>>>> --- a/security/apparmor/include/cred.h
>>>> +++ b/security/apparmor/include/cred.h
>>>> @@ -63,6 +63,26 @@ static inline struct aa_label *aa_get_newest_cred_label(const struct cred *cred)
>>>> return aa_get_newest_label(aa_cred_raw_label(cred));
>>>> }
>>>>
>>>> +static inline struct aa_label *aa_get_newest_cred_label_condref(const struct cred *cred,
>>>> + bool *needput)
>>>> +{
>>>> + struct aa_label *l = aa_cred_raw_label(cred);
>>>> +
>>>> + if (unlikely(label_is_stale(l))) {
>>>> + *needput = true;
>>>> + return aa_get_newest_label(l);
>>>> + }
>>>> +
>>>> + *needput = false;
>>>> + return l;
>>>> +}
>>>> +
>>>> +static inline void aa_put_label_condref(struct aa_label *l, bool needput)
>>>> +{
>>>> + if (unlikely(needput))
>>>> + aa_put_label(l);
>>>> +}
>>>> +
>>>> /**
>>>> * aa_current_raw_label - find the current tasks confining label
>>>> *
>>>> diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
>>>> index 2cea34657a47..4bf87eac4a56 100644
>>>> --- a/security/apparmor/lsm.c
>>>> +++ b/security/apparmor/lsm.c
>>>> @@ -461,6 +461,7 @@ static int apparmor_file_open(struct file *file)
>>>> struct aa_file_ctx *fctx = file_ctx(file);
>>>> struct aa_label *label;
>>>> int error = 0;
>>>> + bool needput;
>>>>
>>>> if (!path_mediated_fs(file->f_path.dentry))
>>>> return 0;
>>>> @@ -477,7 +478,7 @@ static int apparmor_file_open(struct file *file)
>>>> return 0;
>>>> }
>>>>
>>>> - label = aa_get_newest_cred_label(file->f_cred);
>>>> + label = aa_get_newest_cred_label_condref(file->f_cred, &needput);
>>>> if (!unconfined(label)) {
>>>> struct mnt_idmap *idmap = file_mnt_idmap(file);
>>>> struct inode *inode = file_inode(file);
>>>> @@ -494,7 +495,7 @@ static int apparmor_file_open(struct file *file)
>>>> /* todo cache full allowed permissions set and state */
>>>> fctx->allow = aa_map_file_to_perms(file);
>>>> }
>>>> - aa_put_label(label);
>>>> + aa_put_label_condref(label, needput);
>>>>
>>>> return error;
>>>> }
>>
>>
>>
>
Powered by blists - more mailing lists