| Message ID | 20240229174145.3405638-1-meted@linux.ibm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | fanotify: move path permission and security check | expand |
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. I don't really mind the change of outcome, but to me it seems nicer that those tests are inside fanotify_find_path(), so I will want to get a good reason for moving them out. > Move path perm and security checks under the event validation to > prevent this from happening. > Simple reproducer; > > fan_d = fanotify_init(FAN_CLASS_NOTIF, O_RDONLY); > pipe2(pipes, O_CLOEXEC); > fanotify_mark(fan_d, > FAN_MARK_ADD | > FAN_MARK_MOUNT, > FAN_ACCESS, > pipes[0], > NULL); > // expected: EINVAL (22), produces: EACCES (13) > printf("mark errno: %d\n", errno); > > Another reproducer; > ltp/testcases/kernel/syscalls/fanotify/fanotify14 > > Fixes: 69562eb0bd3e ("fanotify: disallow mount/sb marks on kernel internal pseudo fs") > > Signed-off-by: Mete Durlu <meted@linux.ibm.com> > --- > fs/notify/fanotify/fanotify_user.c | 24 +++++++++--------------- > 1 file changed, 9 insertions(+), 15 deletions(-) > > diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c > index fbdc63cc10d9..14121ad0e10d 100644 > --- a/fs/notify/fanotify/fanotify_user.c > +++ b/fs/notify/fanotify/fanotify_user.c > @@ -1015,7 +1015,7 @@ static int fanotify_find_path(int dfd, const char __user *filename, > fdput(f); > goto out; > } > - > + ret = 0; Better convert all gotos in this helper to return. There is nothing in the out label. > *path = f.file->f_path; > path_get(path); > fdput(f); > @@ -1028,21 +1028,7 @@ static int fanotify_find_path(int dfd, const char __user *filename, > lookup_flags |= LOOKUP_DIRECTORY; > > ret = user_path_at(dfd, filename, lookup_flags, path); > - if (ret) > - goto out; > } > - > - /* you can only watch an inode if you have read permissions on it */ > - ret = path_permission(path, MAY_READ); > - if (ret) { > - path_put(path); > - goto out; > - } > - > - ret = security_path_notify(path, mask, obj_type); > - if (ret) > - path_put(path); > - > out: > return ret; > } > @@ -1894,6 +1880,14 @@ static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask, > if (ret) > goto path_put_and_out; > } > + /* you can only watch an inode if you have read permissions on it */ > + ret = path_permission(&path, MAY_READ); > + if (ret) > + goto path_put_and_out; > + > + ret = security_path_notify(&path, mask, obj_type); > + if (ret) > + goto path_put_and_out; > > if (fid_mode) { > ret = fanotify_test_fsid(path.dentry, flags, &__fsid); 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()? The suggested change of behavior seems arbitrary to me. Thanks, Amir.
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. 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. 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. > I don't really mind the change of outcome, but to me it seems > nicer that those tests are inside fanotify_find_path(), so I will > want to get a good reason for moving them out. I agree, when those tests are inside fanotify_find_path() it looks much cleaner but then the check for psuedo fs in fanotify_events_supported() is not made. And I believe when configuring fanotify an EINVAL makes more sense than EACCES, it just seems more informative(at least to me). Would it maybe make sense to put them in a separate helper function, sth like: static int fanotify_path_security(struct path *path, __u64 mask, unsigned int obj_type) { int ret; ret = path_permission(path, MAY_READ); if (ret) return ret; ret = security_path_notify(path, mask, obj_type); return ret; } ... ret = fanotify_path_security(...) if (ret) goto path_put_and_out; >> >> diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c >> index fbdc63cc10d9..14121ad0e10d 100644 >> --- a/fs/notify/fanotify/fanotify_user.c >> +++ b/fs/notify/fanotify/fanotify_user.c >> @@ -1015,7 +1015,7 @@ static int fanotify_find_path(int dfd, const char __user *filename, >> fdput(f); >> goto out; >> } >> - >> + ret = 0; > > Better convert all gotos in this helper to return. > There is nothing in the out label. > Good point, will do! >> *path = f.file->f_path; >> path_get(path); >> fdput(f); >> @@ -1028,21 +1028,7 @@ static int fanotify_find_path(int dfd, const char __user *filename, >> lookup_flags |= LOOKUP_DIRECTORY; >> >> ret = user_path_at(dfd, filename, lookup_flags, path); >> - if (ret) >> - goto out; >> } >> - >> - /* you can only watch an inode if you have read permissions on it */ >> - ret = path_permission(path, MAY_READ); >> - if (ret) { >> - path_put(path); >> - goto out; >> - } >> - >> - ret = security_path_notify(path, mask, obj_type); >> - if (ret) >> - path_put(path); >> - >> out: >> return ret; >> } >> @@ -1894,6 +1880,14 @@ static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask, >> if (ret) >> goto path_put_and_out; >> } >> + /* you can only watch an inode if you have read permissions on it */ >> + ret = path_permission(&path, MAY_READ); >> + if (ret) >> + goto path_put_and_out; >> + >> + ret = security_path_notify(&path, mask, obj_type); >> + if (ret) >> + goto path_put_and_out; >> >> if (fid_mode) { >> ret = fanotify_test_fsid(path.dentry, flags, &__fsid); > > 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. Thank you -Mete Durlu
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. > > 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. > > I don't really mind the change of outcome, but to me it seems > > nicer that those tests are inside fanotify_find_path(), so I will > > want to get a good reason for moving them out. > > I agree, when those tests are inside fanotify_find_path() it looks much > cleaner but then the check for psuedo fs in fanotify_events_supported() > is not made. And I believe when configuring fanotify an EINVAL makes > more sense than EACCES, it just seems more informative(at least to me). > Would it maybe make sense to put them in a separate helper function, > sth like: > > static int fanotify_path_security(struct path *path, > __u64 mask, > unsigned int obj_type) { > int ret; > > ret = path_permission(path, MAY_READ); > if (ret) > return ret; > ret = security_path_notify(path, mask, obj_type); > return ret; > } > *if* we agree that a change to code is needed, then this helper would be very nice. > ... > > ret = fanotify_path_security(...) > if (ret) > goto path_put_and_out; > > >> > >> diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c > >> index fbdc63cc10d9..14121ad0e10d 100644 > >> --- a/fs/notify/fanotify/fanotify_user.c > >> +++ b/fs/notify/fanotify/fanotify_user.c > >> @@ -1015,7 +1015,7 @@ static int fanotify_find_path(int dfd, const char __user *filename, > >> fdput(f); > >> goto out; > >> } > >> - > >> + ret = 0; > > > > Better convert all gotos in this helper to return. > > There is nothing in the out label. > > > Good point, will do! > > >> *path = f.file->f_path; > >> path_get(path); > >> fdput(f); > >> @@ -1028,21 +1028,7 @@ static int fanotify_find_path(int dfd, const char __user *filename, > >> lookup_flags |= LOOKUP_DIRECTORY; > >> > >> ret = user_path_at(dfd, filename, lookup_flags, path); > >> - if (ret) > >> - goto out; > >> } > >> - > >> - /* you can only watch an inode if you have read permissions on it */ > >> - ret = path_permission(path, MAY_READ); > >> - if (ret) { > >> - path_put(path); > >> - goto out; > >> - } > >> - > >> - ret = security_path_notify(path, mask, obj_type); > >> - if (ret) > >> - path_put(path); > >> - > >> out: > >> return ret; > >> } > >> @@ -1894,6 +1880,14 @@ static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask, > >> if (ret) > >> goto path_put_and_out; > >> } > >> + /* you can only watch an inode if you have read permissions on it */ > >> + ret = path_permission(&path, MAY_READ); > >> + if (ret) > >> + goto path_put_and_out; > >> + > >> + ret = security_path_notify(&path, mask, obj_type); > >> + if (ret) > >> + goto path_put_and_out; > >> > >> if (fid_mode) { > >> ret = fanotify_test_fsid(path.dentry, flags, &__fsid); > > > > 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. Thanks, Amir.
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
On Tue, Mar 5, 2024 at 3:57 PM Mete Durlu <meted@linux.ibm.com> wrote: > > 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? LTP patch sounds good to me. How to fix the test to support SELINUX it will be up to LTP maintainers to comment. Thanks, Amir.
diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c index fbdc63cc10d9..14121ad0e10d 100644 --- a/fs/notify/fanotify/fanotify_user.c +++ b/fs/notify/fanotify/fanotify_user.c @@ -1015,7 +1015,7 @@ static int fanotify_find_path(int dfd, const char __user *filename, fdput(f); goto out; } - + ret = 0; *path = f.file->f_path; path_get(path); fdput(f); @@ -1028,21 +1028,7 @@ static int fanotify_find_path(int dfd, const char __user *filename, lookup_flags |= LOOKUP_DIRECTORY; ret = user_path_at(dfd, filename, lookup_flags, path); - if (ret) - goto out; } - - /* you can only watch an inode if you have read permissions on it */ - ret = path_permission(path, MAY_READ); - if (ret) { - path_put(path); - goto out; - } - - ret = security_path_notify(path, mask, obj_type); - if (ret) - path_put(path); - out: return ret; } @@ -1894,6 +1880,14 @@ static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask, if (ret) goto path_put_and_out; } + /* you can only watch an inode if you have read permissions on it */ + ret = path_permission(&path, MAY_READ); + if (ret) + goto path_put_and_out; + + ret = security_path_notify(&path, mask, obj_type); + if (ret) + goto path_put_and_out; if (fid_mode) { ret = fanotify_test_fsid(path.dentry, flags, &__fsid);
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. Move path perm and security checks under the event validation to prevent this from happening. Simple reproducer; fan_d = fanotify_init(FAN_CLASS_NOTIF, O_RDONLY); pipe2(pipes, O_CLOEXEC); fanotify_mark(fan_d, FAN_MARK_ADD | FAN_MARK_MOUNT, FAN_ACCESS, pipes[0], NULL); // expected: EINVAL (22), produces: EACCES (13) printf("mark errno: %d\n", errno); Another reproducer; ltp/testcases/kernel/syscalls/fanotify/fanotify14 Fixes: 69562eb0bd3e ("fanotify: disallow mount/sb marks on kernel internal pseudo fs") Signed-off-by: Mete Durlu <meted@linux.ibm.com> --- fs/notify/fanotify/fanotify_user.c | 24 +++++++++--------------- 1 file changed, 9 insertions(+), 15 deletions(-)