smack: Possible NULL pointer deref in cred_free hook.

[Casey Schaufler]

On 2/7/2024 10:53 AM, Casey Schaufler wrote:
> On 2/6/2024 6:54 PM, Tetsuo Handa wrote:
>> On 2024/02/07 10:39, Casey Schaufler wrote:
>>> On 2/6/2024 6:31 AM, Tetsuo Handa wrote:
>>>> Hello, Casey.
>>>>
>>>> I confirmed using fault injection shown below that smack_cred_free() is not
>>>> prepared for being called without successful smack_cred_prepare().
>>> The failure cases for smack_cred_prepare() result from memory allocation
>>> failures. Since init_task_smack() is called before either of the potential
>>> memory allocations the state of the cred will be safe for smack_cred_free().
>>> The fault you've described here removes the init_task_smack(), which will
>>> always succeed, and which is sufficient to prevent the smack_cred_free()
>>> failure below. Are you suggesting that there is a case where a cred will
>>> be freed without ever having been "prepared"?
>> Yes. If smack_cred_prepare() is not the first entry of the cred_prepare list
>> and the first entry of the cred_prepare list failed, smack_cred_prepare()
>> will not be called (and therefore init_task_smack() will not be called).

Ah, but it turns out that the only LSM that can fail in _cred_prepare()
is Smack. Even if smack_cred_prepare() fails it will have called
init_task_smack(), so there isn't *currently* a problem. Should another
LSM have the possibility of failing in whatever_cred_prepare() this
could be an issue.

Your "fault injection" is too aggressive. It should return an error
from smack_cred_prepare() after the call to init_task_smack() rather
than commenting out the call entirely.

> I see your point. Thank you for the insight. This is the first real
> case I've seen where the "bail on fail" approach leads to a problem.
> Now, on to the fix ...
>
>