[apparmor] AppArmor kernel audit locks up system

[John Johansen]

On 10/9/23 10:06, Paul Moore wrote:
> On Mon, Oct 9, 2023 at 2:40 AM Andreas Steinmetz
> <anstein99 at googlemail.com> wrote:
>> On Sat, Oct 7, 2023 at 12:07 AM Paul Moore <paul at paul-moore.com> wrote:
>>>
>>> Does anyone else have any bright ideas or crazy thoughts on this?
>>>
>>
>> Well, not really an idea and for sure either crazy or dumb:
>>
>> Why not use the data already available from DEFINE_AUDIT_DATA() to
>> determine the call path (or add a modifiable field to the struct) and
>> handle locking accordingly?
> 
> It's possible I'm missing something as I'm not very familiar with the
> AppArmor details, but I'm not sure how this would solve the problem;
> can you elaborate on this?
> 

it doesn't. We might be able to use it to paper over the issue in
apparmor by not issuing an audit message for the hook in question, but
that doesn't fix the problem.

To be clear the issue is that if there is an audit exec filter rule load
and any LSM sends an audit message from the capable or security_task_setrlimit()
hooks, the task lock is taken recursively causing a lockup.

>> Anyway, this problem can be seen as a DoS vector. Any malicious code
>> could trigger some audit causing a system lockup. So however ugly the
>> solution this needs to be solved.
> 
> I don't think anyone is objecting to resolving this, it's more a
> matter of *how* we can resolve it.
> 

currently I am see four crazy/stupid paths forward, each with their own
pain points.


1. lift the capable() and security_task_setrlimit() calls out of the lock.

this might be possible, it should be fine for capable() but does open
a potential race window for security_task_setrlimit() if the LSM hooks
mediation looks at the tasks current resource value.

2. rework get_task_exe_file() to not need the task lock. That looks
like a major under taking, and I don't currently see it as viable.

3. get the task lock switch to a recursive spin_lock. Another large
piece of work that I don't currently see as viable.

4. caching a reference in the audit_context as paul has suggested.