Re: [PATCH] fanotify: move path permission and security check

[Mete Durlu <meted@linux.ibm.com>]

On 3/2/24 10:58, Amir Goldstein wrote:
> On Fri, Mar 1, 2024 at 3:16 PM Mete Durlu <meted@linux.ibm.com> wrote:
>>
>> On 3/1/24 10:52, Amir Goldstein wrote:
>>> On Thu, Feb 29, 2024 at 7:53 PM Mete Durlu <meted@linux.ibm.com> wrote:
>>>>
>>>> In current state do_fanotify_mark() does path permission and security
>>>> checking before doing the event configuration checks. In the case
>>>> where user configures mount and sb marks with kernel internal pseudo
>>>> fs, security_path_notify() yields an EACESS and causes an earlier
>>>> exit. Instead, this particular case should have been handled by
>>>> fanotify_events_supported() and exited with an EINVAL.
>>>
>>> What makes you say that this is the expected outcome?
>>> I'd say that the expected outcome is undefined and we have no reason
>>> to commit to either  EACCESS or EINVAL outcome.
>>
>> TLDR; I saw the failing ltp test(fanotify14) started investigating, read
>> the comments on the related commits and noticed that the fanotify
>> documentation does not mention any EACESS as an errno. For these reasons
>> I made an attempt to provide a fix. The placement of the checks aim
>> minimal change, I just tried not to alter the logic more than needed.
>> Thanks for the feedback, will apply suggestions.
>>
> 
> Generally speaking, the reasons above themselves are good enough
> reasons for fixing the documentation and fixing the test code, but not
> enough reasons to change the code.
> 
> There may be other good reasons for changing the code, but I am not
> sure they apply here.
>

I understand the concerns and the reasoning. My findings and suggestions
are below.

>>
>> The main reason is the following commit;
>> * linux: 69562eb0bd3e ("fanotify: disallow mount/sb marks on kernel
>> internal pseudo fs")
>>
>> fanotify_user: fanotify_events_supported()
>>       /*
>>        * mount and sb marks are not allowed on kernel internal pseudo
>>            * fs, like pipe_mnt, because that would subscribe to events on
>>            * all the anonynous pipes in the system.
>>        */
>>       if (mark_type != FAN_MARK_INODE &&
>>           path->mnt->mnt_sb->s_flags & SB_NOUSER)
>>           return -EINVAL;
>>
>> It looks to me as, when configuring fanotify_mark with pipes and
>> FAN_MARK_MOUNT or FAN_MARK_FILESYSTEM, the path above should be taken
>> instead of an early return with EACCES.
>>
> 
> It is a subjective opinion. I do not agree with it, but it does not matter if
> I agree with this statement or not, what matters it that there is no clear
> definition across system calls of what SHOULD happen in this case
> and IMO there is no reason for us to commit to this behavior or the other.
> 
>> Also the following commit on linux test project(ltp) expects EINVAL as
>> expected errno.
>>
>> * ltp: 8e897008c ("fanotify14: Test disallow sb/mount mark on anonymous
>> pipe")
>>
>> To be honest, the test added on above commit is the main reason why I
>> started investigating this.
>>
> 
> This is something that I don't understand.
> If you are running LTP in a setup that rejects fanotify_mark() due to
> security policy, how do the rest of the fanotify tests pass?
> I feel like I am missing information about the test regression report.
> I never test with a security policy applied so I have no idea what
> might be expected.
> 
Ah, I always run with defconfig which has SELINUX enabled and by default
SELINUX is configured to `enforcing` (so far I tested with x86 and s390x
but a quick grep shows most other architectures also have it enabled on
their defconfigs). With SELINUX enabled LTP's fanotify14 shows failures
on

fanotify14.c:284: TINFO: Testing FAN_MARK_MOUNT with anonymous pipe
fanotify14.c:284: TINFO: Testing FAN_MARK_FILESYSTEM with anonymous pipe

since they return -EACCES instead of -EINVAL.Other test cases pass.
Once I disable SELINUX, ALL test cases pass.

>>>
>>> If we do accept your argument that security_path_notify() should be
>>> after fanotify_events_supported(). Why not also after fanotify_test_fsid()
>>> and fanotify_test_fid()?
>>
>> I tried to place the checks as close as possible to their original
>> position, that is why I placed them right after
>> fanotify_events_supported(). I wanted to keep the ordering as close as
>> possible to original to not break any other check. I am open to
>> suggestions regarding this.
>>
> 
> It is a matter of principle IMO.
> If you argue that access permission errors have priority over validity
> of API arguments, then  fanotify_test_{fsid,fid}() are not that much
> different (priority-wise) from fanotify_events_supported().
> 
> My preference is to not change the code, but maybe Jan will
> have a different opinion.

I understand the argument, then I propose patching the LTP and appending
the documentation. My first idea is to send a patch for LTP so that,
fanotify14 could check if SELINUX is enforcing and change the testcases
expected errno accordingly. How does that sound?

Thank you.
-Mete Durlu