| Message ID | 20241231014632.589049-1-enlightened@chromium.org (mailing list archive) |
|---|---|
| State | Under Review |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | [1/2] fs: add loopback/bind mount specific security hook | expand |
Hi Shervin, kernel test robot noticed the following build errors: [auto build test ERROR on fc033cf25e612e840e545f8d5ad2edd6ba613ed5] url: https://github.com/intel-lab-lkp/linux/commits/Shervin-Oloumi/landlock-add-support-for-private-bind-mount/20241231-094806 base: fc033cf25e612e840e545f8d5ad2edd6ba613ed5 patch link: https://lore.kernel.org/r/20241231014632.589049-1-enlightened%40chromium.org patch subject: [PATCH 1/2] fs: add loopback/bind mount specific security hook config: sparc-randconfig-002-20241231 (https://download.01.org/0day-ci/archive/20241231/202412311303.Cb16SNL3-lkp@intel.com/config) compiler: sparc-linux-gcc (GCC) 14.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241231/202412311303.Cb16SNL3-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202412311303.Cb16SNL3-lkp@intel.com/ All errors (new ones prefixed by >>): fs/namespace.c: In function 'do_loopback': >> fs/namespace.c:2768:15: error: implicit declaration of function 'security_sb_bindmount'; did you mean 'security_sb_umount'? [-Wimplicit-function-declaration] 2768 | err = security_sb_bindmount(&old_path, path); | ^~~~~~~~~~~~~~~~~~~~~ | security_sb_umount vim +2768 fs/namespace.c 2751 2752 /* 2753 * do loopback mount. 2754 */ 2755 static int do_loopback(struct path *path, const char *old_name, 2756 int recurse) 2757 { 2758 struct path old_path; 2759 struct mount *mnt = NULL, *parent; 2760 struct mountpoint *mp; 2761 int err; 2762 if (!old_name || !*old_name) 2763 return -EINVAL; 2764 err = kern_path(old_name, LOOKUP_FOLLOW|LOOKUP_AUTOMOUNT, &old_path); 2765 if (err) 2766 return err; 2767 > 2768 err = security_sb_bindmount(&old_path, path); 2769 if (err) 2770 goto out; 2771 2772 err = -EINVAL; 2773 if (mnt_ns_loop(old_path.dentry)) 2774 goto out; 2775 2776 mp = lock_mount(path); 2777 if (IS_ERR(mp)) { 2778 err = PTR_ERR(mp); 2779 goto out; 2780 } 2781 2782 parent = real_mount(path->mnt); 2783 if (!check_mnt(parent)) 2784 goto out2; 2785 2786 mnt = __do_loopback(&old_path, recurse); 2787 if (IS_ERR(mnt)) { 2788 err = PTR_ERR(mnt); 2789 goto out2; 2790 } 2791 2792 err = graft_tree(mnt, parent, mp); 2793 if (err) { 2794 lock_mount_hash(); 2795 umount_tree(mnt, UMOUNT_SYNC); 2796 unlock_mount_hash(); 2797 } 2798 out2: 2799 unlock_mount(mp); 2800 out: 2801 path_put(&old_path); 2802 return err; 2803 } 2804
Hi Shervin, kernel test robot noticed the following build errors: [auto build test ERROR on fc033cf25e612e840e545f8d5ad2edd6ba613ed5] url: https://github.com/intel-lab-lkp/linux/commits/Shervin-Oloumi/landlock-add-support-for-private-bind-mount/20241231-094806 base: fc033cf25e612e840e545f8d5ad2edd6ba613ed5 patch link: https://lore.kernel.org/r/20241231014632.589049-1-enlightened%40chromium.org patch subject: [PATCH 1/2] fs: add loopback/bind mount specific security hook config: um-randconfig-001-20241231 (https://download.01.org/0day-ci/archive/20241231/202412311322.DkS3TbED-lkp@intel.com/config) compiler: clang version 20.0.0git (https://github.com/llvm/llvm-project 319b89197348b7cad1215e235bdc7b5ec8f9b72c) reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241231/202412311322.DkS3TbED-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202412311322.DkS3TbED-lkp@intel.com/ All errors (new ones prefixed by >>): >> fs/namespace.c:2768:8: error: call to undeclared function 'security_sb_bindmount'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration] 2768 | err = security_sb_bindmount(&old_path, path); | ^ 1 error generated. vim +/security_sb_bindmount +2768 fs/namespace.c 2751 2752 /* 2753 * do loopback mount. 2754 */ 2755 static int do_loopback(struct path *path, const char *old_name, 2756 int recurse) 2757 { 2758 struct path old_path; 2759 struct mount *mnt = NULL, *parent; 2760 struct mountpoint *mp; 2761 int err; 2762 if (!old_name || !*old_name) 2763 return -EINVAL; 2764 err = kern_path(old_name, LOOKUP_FOLLOW|LOOKUP_AUTOMOUNT, &old_path); 2765 if (err) 2766 return err; 2767 > 2768 err = security_sb_bindmount(&old_path, path); 2769 if (err) 2770 goto out; 2771 2772 err = -EINVAL; 2773 if (mnt_ns_loop(old_path.dentry)) 2774 goto out; 2775 2776 mp = lock_mount(path); 2777 if (IS_ERR(mp)) { 2778 err = PTR_ERR(mp); 2779 goto out; 2780 } 2781 2782 parent = real_mount(path->mnt); 2783 if (!check_mnt(parent)) 2784 goto out2; 2785 2786 mnt = __do_loopback(&old_path, recurse); 2787 if (IS_ERR(mnt)) { 2788 err = PTR_ERR(mnt); 2789 goto out2; 2790 } 2791 2792 err = graft_tree(mnt, parent, mp); 2793 if (err) { 2794 lock_mount_hash(); 2795 umount_tree(mnt, UMOUNT_SYNC); 2796 unlock_mount_hash(); 2797 } 2798 out2: 2799 unlock_mount(mp); 2800 out: 2801 path_put(&old_path); 2802 return err; 2803 } 2804
On 12/30/2024 5:46 PM, Shervin Oloumi wrote: > The main mount security hook (security_sb_mount) is called early in the > process before the mount type is determined and the arguments are > validated and converted to the appropriate format. Specifically, the > source path is surfaced as a string, which is not appropriate for > checking bind mount requests. For bind mounts the source should be > validated and passed as a path struct (same as destination), after the > mount type is determined. This allows the hook users to evaluate the > mount attributes without the need to perform any validations or > conversions out of band, which can introduce a TOCTOU race condition. > > The newly introduced hook is invoked only if the security_sb_mount hook > passes, and only if the MS_BIND flag is detected. At this point the > source of the mount has been successfully converted to a path struct > using the kernel's kern_path API. This allows LSMs to target bind mount > requests at the right stage, and evaluate the attributes in the right > format, based on the type of mount. I am not very happy about an LSM hook that is this specific. Why restrict it to bind mounts? It seems that there might be other valuable restrictions on mounts that are based on the path. > > This does not affect the functionality of the existing mount security > hooks, including security_sb_mount. The new hook, can be utilized as a > supplement to the main hook for further analyzing bind mount requests. > This means that there is still the option of only using the main hook > function, if all one wants to do is indiscriminately reject all bind > mount requests, regardless of the source and destination arguments. > However, if one needs to evaluate the source and destination of a bind > mount request before making a decision, this hook function should be > preferred. Of course, if a bind mount request does not make it past the > security_sb_mount check, the bind mount hook function is never invoked. > > Signed-off-by: Shervin Oloumi <enlightened@chromium.org> > --- > fs/namespace.c | 4 ++++ > include/linux/lsm_hook_defs.h | 1 + > include/linux/security.h | 1 + > security/security.c | 16 ++++++++++++++++ > 4 files changed, 22 insertions(+) > > diff --git a/fs/namespace.c b/fs/namespace.c > index 23e81c2a1e3f..c902608c9759 100644 > --- a/fs/namespace.c > +++ b/fs/namespace.c > @@ -2765,6 +2765,10 @@ static int do_loopback(struct path *path, const char *old_name, > if (err) > return err; > > + err = security_sb_bindmount(&old_path, path); I can easily envision uses for this other than bind mounts. Perhaps security_sb_mount_path()? > + if (err) > + goto out; > + > err = -EINVAL; > if (mnt_ns_loop(old_path.dentry)) > goto out; > diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h > index eb2937599cb0..3d1940239556 100644 > --- a/include/linux/lsm_hook_defs.h > +++ b/include/linux/lsm_hook_defs.h > @@ -71,6 +71,7 @@ LSM_HOOK(int, 0, sb_show_options, struct seq_file *m, struct super_block *sb) > LSM_HOOK(int, 0, sb_statfs, struct dentry *dentry) > LSM_HOOK(int, 0, sb_mount, const char *dev_name, const struct path *path, > const char *type, unsigned long flags, void *data) > +LSM_HOOK(int, 0, sb_bindmount, const struct path *old_path, const struct path *path) > LSM_HOOK(int, 0, sb_umount, struct vfsmount *mnt, int flags) > LSM_HOOK(int, 0, sb_pivotroot, const struct path *old_path, > const struct path *new_path) > diff --git a/include/linux/security.h b/include/linux/security.h > index cbdba435b798..512ac656500e 100644 > --- a/include/linux/security.h > +++ b/include/linux/security.h > @@ -365,6 +365,7 @@ int security_sb_show_options(struct seq_file *m, struct super_block *sb); > int security_sb_statfs(struct dentry *dentry); > int security_sb_mount(const char *dev_name, const struct path *path, > const char *type, unsigned long flags, void *data); > +int security_sb_bindmount(const struct path *old_path, const struct path *path); > int security_sb_umount(struct vfsmount *mnt, int flags); > int security_sb_pivotroot(const struct path *old_path, const struct path *new_path); > int security_sb_set_mnt_opts(struct super_block *sb, > diff --git a/security/security.c b/security/security.c > index 09664e09fec9..bd7cb3df16f4 100644 > --- a/security/security.c > +++ b/security/security.c > @@ -1564,6 +1564,22 @@ int security_sb_mount(const char *dev_name, const struct path *path, > return call_int_hook(sb_mount, dev_name, path, type, flags, data); > } > > +/** > + * security_sb_bindmount() - Loopback/bind mount specific permission check > + * @old_path: source of loopback/bind mount > + * @path: mount point > + * > + * This check is performed in addition to security_sb_mount and only if the > + * mount type is determined to be loopback/bind mount (flags & MS_BIND). It > + * surfaces the mount source as a path struct. > + * > + * Return: Returns 0 if permission is granted. > + */ > +int security_sb_bindmount(const struct path *old_path, const struct path *path) > +{ > + return call_int_hook(sb_bindmount, old_path, path); > +} > + > /** > * security_sb_umount() - Check permission for unmounting a filesystem > * @mnt: mounted filesystem > > base-commit: fc033cf25e612e840e545f8d5ad2edd6ba613ed5
On Mon, Dec 30, 2024 at 8:46 PM Shervin Oloumi <enlightened@chromium.org> wrote: > > The main mount security hook (security_sb_mount) is called early in the > process before the mount type is determined and the arguments are > validated and converted to the appropriate format. Specifically, the > source path is surfaced as a string, which is not appropriate for > checking bind mount requests. For bind mounts the source should be > validated and passed as a path struct (same as destination), after the > mount type is determined. This allows the hook users to evaluate the > mount attributes without the need to perform any validations or > conversions out of band, which can introduce a TOCTOU race condition. > > The newly introduced hook is invoked only if the security_sb_mount hook > passes, and only if the MS_BIND flag is detected. At this point the > source of the mount has been successfully converted to a path struct > using the kernel's kern_path API. This allows LSMs to target bind mount > requests at the right stage, and evaluate the attributes in the right > format, based on the type of mount. > > This does not affect the functionality of the existing mount security > hooks, including security_sb_mount. The new hook, can be utilized as a > supplement to the main hook for further analyzing bind mount requests. > This means that there is still the option of only using the main hook > function, if all one wants to do is indiscriminately reject all bind > mount requests, regardless of the source and destination arguments. > However, if one needs to evaluate the source and destination of a bind > mount request before making a decision, this hook function should be > preferred. Of course, if a bind mount request does not make it past the > security_sb_mount check, the bind mount hook function is never invoked. > > Signed-off-by: Shervin Oloumi <enlightened@chromium.org> > --- > fs/namespace.c | 4 ++++ > include/linux/lsm_hook_defs.h | 1 + > include/linux/security.h | 1 + > security/security.c | 16 ++++++++++++++++ > 4 files changed, 22 insertions(+) Like Casey I'm not really excited about such a specific LSM hook, but unfortunately we can't simply call kern_path() in the existing security_sb_mount() callback as that could end up resolving to something different than the call in do_loopback() which would be bad. Moving the kern_path() call up into path_mount() just looks like a bad idea anyway you look at it. Unfortunately I don't really see an alternative to what you're proposing, so I guess we're kinda stuck with this as a solution, unless someone can think of something better. I'm going to need to see an ACK from the VFS folks on this before I merge the new hook. I'd also stick with the security_sb_bindmount() name as opposed to the XXX_path() suggestion from Casey simply to help distinguish it from the pathname based LSM hooks. Yes, this is operating on the pathname, but bind mounts are a bit of a special case. Unrelated, but I just noticed that we are calling security_sb_mount() *before* may_mount(); that's the opposite order for most LSM hook placements where we do the discretionary/capabilities checks first and the LSM checks. That's something we should look at, perhaps there is a good reason for the ordering being different, perhaps it's a mistake. > diff --git a/fs/namespace.c b/fs/namespace.c > index 23e81c2a1e3f..c902608c9759 100644 > --- a/fs/namespace.c > +++ b/fs/namespace.c > @@ -2765,6 +2765,10 @@ static int do_loopback(struct path *path, const char *old_name, > if (err) > return err; > > + err = security_sb_bindmount(&old_path, path); > + if (err) > + goto out; I might make a mention in the commit description that the do_reconfigure_mnt() case (MS_REMOUNT|MS_BIND) should be able to be handled using the existing security_sb_mount() hook. > err = -EINVAL; > if (mnt_ns_loop(old_path.dentry)) > goto out; ... > diff --git a/security/security.c b/security/security.c > index 09664e09fec9..bd7cb3df16f4 100644 > --- a/security/security.c > +++ b/security/security.c > @@ -1564,6 +1564,22 @@ int security_sb_mount(const char *dev_name, const struct path *path, > return call_int_hook(sb_mount, dev_name, path, type, flags, data); > } > > +/** > + * security_sb_bindmount() - Loopback/bind mount specific permission check > + * @old_path: source of loopback/bind mount > + * @path: mount point > + * > + * This check is performed in addition to security_sb_mount and only if the > + * mount type is determined to be loopback/bind mount (flags & MS_BIND). It > + * surfaces the mount source as a path struct. I wouldn't mention security_sb_mount() above as that makes the comment somewhat fragile in the face of changing hooks. I would suggest something along these lines: "Beyond any general mounting hooks, this check is performed on an initial loopback/bind mount (MS_BIND) with the mount source presented as a path struct in @old_path." > + * Return: Returns 0 if permission is granted. > + */ > +int security_sb_bindmount(const struct path *old_path, const struct path *path) > +{ > + return call_int_hook(sb_bindmount, old_path, path); > +} > + > /** > * security_sb_umount() - Check permission for unmounting a filesystem > * @mnt: mounted filesystem > > base-commit: fc033cf25e612e840e545f8d5ad2edd6ba613ed5 > -- > 2.47.1.613.gc27f4b7a9f-goog
On Mon 30-12-24 17:46:31, Shervin Oloumi wrote: > The main mount security hook (security_sb_mount) is called early in the > process before the mount type is determined and the arguments are > validated and converted to the appropriate format. Specifically, the > source path is surfaced as a string, which is not appropriate for > checking bind mount requests. For bind mounts the source should be > validated and passed as a path struct (same as destination), after the > mount type is determined. This allows the hook users to evaluate the > mount attributes without the need to perform any validations or > conversions out of band, which can introduce a TOCTOU race condition. > > The newly introduced hook is invoked only if the security_sb_mount hook > passes, and only if the MS_BIND flag is detected. At this point the > source of the mount has been successfully converted to a path struct > using the kernel's kern_path API. This allows LSMs to target bind mount > requests at the right stage, and evaluate the attributes in the right > format, based on the type of mount. > > This does not affect the functionality of the existing mount security > hooks, including security_sb_mount. The new hook, can be utilized as a > supplement to the main hook for further analyzing bind mount requests. > This means that there is still the option of only using the main hook > function, if all one wants to do is indiscriminately reject all bind > mount requests, regardless of the source and destination arguments. > However, if one needs to evaluate the source and destination of a bind > mount request before making a decision, this hook function should be > preferred. Of course, if a bind mount request does not make it past the > security_sb_mount check, the bind mount hook function is never invoked. > > Signed-off-by: Shervin Oloumi <enlightened@chromium.org> Christian is much more experienced in this area than me but let me share my thoughts before he returns from vacation. > diff --git a/fs/namespace.c b/fs/namespace.c > index 23e81c2a1e3f..c902608c9759 100644 > --- a/fs/namespace.c > +++ b/fs/namespace.c > @@ -2765,6 +2765,10 @@ static int do_loopback(struct path *path, const char *old_name, > if (err) > return err; > > + err = security_sb_bindmount(&old_path, path); > + if (err) > + goto out; > + So this gets triggered for the legacy mount API path (mount(2) syscall). For the new mount API, I can see open_tree() does not have any security hook. Is that intented? Are you catching equivalent changes done through the new mount API inside security_move_mount() hook? Also what caught my eye is that the LSM doesn't care at all whether this is a recursive bind mount (copying all mounts beneath the specified one) or just a normal one (copying only a single mount). Maybe that's fine but it seems a bit counter-intuitive to me as AFAIU it implicitly places a requirement on the policy that if doing some bind mount is forbidden, then doing bind mount of any predecessor must be forbidden as well (otherwise the policy will be inconsistent). Honza
Thanks for the feedback Paul, the latest patch should include the recommendations now. On Thu, Jan 2, 2025 at 9:11 PM Paul Moore <paul@paul-moore.com> wrote: > > On Mon, Dec 30, 2024 at 8:46 PM Shervin Oloumi <enlightened@chromium.org> wrote: > > > > The main mount security hook (security_sb_mount) is called early in the > > process before the mount type is determined and the arguments are > > validated and converted to the appropriate format. Specifically, the > > source path is surfaced as a string, which is not appropriate for > > checking bind mount requests. For bind mounts the source should be > > validated and passed as a path struct (same as destination), after the > > mount type is determined. This allows the hook users to evaluate the > > mount attributes without the need to perform any validations or > > conversions out of band, which can introduce a TOCTOU race condition. > > > > The newly introduced hook is invoked only if the security_sb_mount hook > > passes, and only if the MS_BIND flag is detected. At this point the > > source of the mount has been successfully converted to a path struct > > using the kernel's kern_path API. This allows LSMs to target bind mount > > requests at the right stage, and evaluate the attributes in the right > > format, based on the type of mount. > > > > This does not affect the functionality of the existing mount security > > hooks, including security_sb_mount. The new hook, can be utilized as a > > supplement to the main hook for further analyzing bind mount requests. > > This means that there is still the option of only using the main hook > > function, if all one wants to do is indiscriminately reject all bind > > mount requests, regardless of the source and destination arguments. > > However, if one needs to evaluate the source and destination of a bind > > mount request before making a decision, this hook function should be > > preferred. Of course, if a bind mount request does not make it past the > > security_sb_mount check, the bind mount hook function is never invoked. > > > > Signed-off-by: Shervin Oloumi <enlightened@chromium.org> > > --- > > fs/namespace.c | 4 ++++ > > include/linux/lsm_hook_defs.h | 1 + > > include/linux/security.h | 1 + > > security/security.c | 16 ++++++++++++++++ > > 4 files changed, 22 insertions(+) > > Like Casey I'm not really excited about such a specific LSM hook, but > unfortunately we can't simply call kern_path() in the existing > security_sb_mount() callback as that could end up resolving to > something different than the call in do_loopback() which would be bad. > Moving the kern_path() call up into path_mount() just looks like a bad > idea anyway you look at it. Unfortunately I don't really see an > alternative to what you're proposing, so I guess we're kinda stuck > with this as a solution, unless someone can think of something better. > > I'm going to need to see an ACK from the VFS folks on this before I > merge the new hook. > > I'd also stick with the security_sb_bindmount() name as opposed to the > XXX_path() suggestion from Casey simply to help distinguish it from > the pathname based LSM hooks. Yes, this is operating on the pathname, > but bind mounts are a bit of a special case. > > Unrelated, but I just noticed that we are calling security_sb_mount() > *before* may_mount(); that's the opposite order for most LSM hook > placements where we do the discretionary/capabilities checks first and > the LSM checks. That's something we should look at, perhaps there is > a good reason for the ordering being different, perhaps it's a > mistake. > > > diff --git a/fs/namespace.c b/fs/namespace.c > > index 23e81c2a1e3f..c902608c9759 100644 > > --- a/fs/namespace.c > > +++ b/fs/namespace.c > > @@ -2765,6 +2765,10 @@ static int do_loopback(struct path *path, const char *old_name, > > if (err) > > return err; > > > > + err = security_sb_bindmount(&old_path, path); > > + if (err) > > + goto out; > > I might make a mention in the commit description that the > do_reconfigure_mnt() case (MS_REMOUNT|MS_BIND) should be able to be > handled using the existing security_sb_mount() hook. > > > err = -EINVAL; > > if (mnt_ns_loop(old_path.dentry)) > > goto out; > > ... > > > diff --git a/security/security.c b/security/security.c > > index 09664e09fec9..bd7cb3df16f4 100644 > > --- a/security/security.c > > +++ b/security/security.c > > @@ -1564,6 +1564,22 @@ int security_sb_mount(const char *dev_name, const struct path *path, > > return call_int_hook(sb_mount, dev_name, path, type, flags, data); > > } > > > > +/** > > + * security_sb_bindmount() - Loopback/bind mount specific permission check > > + * @old_path: source of loopback/bind mount > > + * @path: mount point > > + * > > + * This check is performed in addition to security_sb_mount and only if the > > + * mount type is determined to be loopback/bind mount (flags & MS_BIND). It > > + * surfaces the mount source as a path struct. > > I wouldn't mention security_sb_mount() above as that makes the comment > somewhat fragile in the face of changing hooks. I would suggest > something along these lines: > > "Beyond any general mounting hooks, this check is performed on an > initial loopback/bind mount (MS_BIND) with the mount source presented > as a path struct in @old_path." > > > + * Return: Returns 0 if permission is granted. > > + */ > > +int security_sb_bindmount(const struct path *old_path, const struct path *path) > > +{ > > + return call_int_hook(sb_bindmount, old_path, path); > > +} > > + > > /** > > * security_sb_umount() - Check permission for unmounting a filesystem > > * @mnt: mounted filesystem > > > > base-commit: fc033cf25e612e840e545f8d5ad2edd6ba613ed5 > > -- > > 2.47.1.613.gc27f4b7a9f-goog > > -- > paul-moore.com
On Fri, Jan 3, 2025 at 3:11 AM Jan Kara <jack@suse.cz> wrote: > > On Mon 30-12-24 17:46:31, Shervin Oloumi wrote: > > The main mount security hook (security_sb_mount) is called early in the > > process before the mount type is determined and the arguments are > > validated and converted to the appropriate format. Specifically, the > > source path is surfaced as a string, which is not appropriate for > > checking bind mount requests. For bind mounts the source should be > > validated and passed as a path struct (same as destination), after the > > mount type is determined. This allows the hook users to evaluate the > > mount attributes without the need to perform any validations or > > conversions out of band, which can introduce a TOCTOU race condition. > > > > The newly introduced hook is invoked only if the security_sb_mount hook > > passes, and only if the MS_BIND flag is detected. At this point the > > source of the mount has been successfully converted to a path struct > > using the kernel's kern_path API. This allows LSMs to target bind mount > > requests at the right stage, and evaluate the attributes in the right > > format, based on the type of mount. > > > > This does not affect the functionality of the existing mount security > > hooks, including security_sb_mount. The new hook, can be utilized as a > > supplement to the main hook for further analyzing bind mount requests. > > This means that there is still the option of only using the main hook > > function, if all one wants to do is indiscriminately reject all bind > > mount requests, regardless of the source and destination arguments. > > However, if one needs to evaluate the source and destination of a bind > > mount request before making a decision, this hook function should be > > preferred. Of course, if a bind mount request does not make it past the > > security_sb_mount check, the bind mount hook function is never invoked. > > > > Signed-off-by: Shervin Oloumi <enlightened@chromium.org> > > Christian is much more experienced in this area than me but let me share my > thoughts before he returns from vacation. > > > diff --git a/fs/namespace.c b/fs/namespace.c > > index 23e81c2a1e3f..c902608c9759 100644 > > --- a/fs/namespace.c > > +++ b/fs/namespace.c > > @@ -2765,6 +2765,10 @@ static int do_loopback(struct path *path, const char *old_name, > > if (err) > > return err; > > > > + err = security_sb_bindmount(&old_path, path); > > + if (err) > > + goto out; > > + > > So this gets triggered for the legacy mount API path (mount(2) syscall). > For the new mount API, I can see open_tree() does not have any security > hook. Is that intented? Are you catching equivalent changes done through > the new mount API inside security_move_mount() hook? I am not very familiar with the new API and when it is used, but LandLock does listen to security_move_mount() and it rejects all such requests. It also listens to and rejects remount and pivotroot. Are there cases where bind mount requests go through open_tree() and this hook is bypassed? > Also what caught my eye is that the LSM doesn't care at all whether this is > a recursive bind mount (copying all mounts beneath the specified one) or > just a normal one (copying only a single mount). Maybe that's fine but it > seems a bit counter-intuitive to me as AFAIU it implicitly places a > requirement on the policy that if doing some bind mount is forbidden, then > doing bind mount of any predecessor must be forbidden as well (otherwise > the policy will be inconsistent). I need to double check this with Mickaël, but I think it is safe to allow recursive bind mounts. If a bind mount was allowed, let's say /A -> /home/B, then we already verified that the process did not gain more access (or even dropped some access) through the new mount point, e.g. accessing /A/a through /home/B/a. So with a recursive bind mount, let's say /home -> /C, once we check for privilege escalation as usual, it should be safe to clone any existing sub mounts in the new destination. Because if access through B <= A and C <= B then access through C <= A. So back to your point, there should never exist an illegal bind mount action that can implicitly happen through a legal recursive bind mount of its predecessor. Regardless, I think it might be useful to know if a mount is recursive for other use cases so I added a boolean for surfacing MS_REC in the new patches. > > Honza > -- > Jan Kara <jack@suse.com> > SUSE Labs, CR
diff --git a/fs/namespace.c b/fs/namespace.c index 23e81c2a1e3f..c902608c9759 100644 --- a/fs/namespace.c +++ b/fs/namespace.c @@ -2765,6 +2765,10 @@ static int do_loopback(struct path *path, const char *old_name, if (err) return err; + err = security_sb_bindmount(&old_path, path); + if (err) + goto out; + err = -EINVAL; if (mnt_ns_loop(old_path.dentry)) goto out; diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h index eb2937599cb0..3d1940239556 100644 --- a/include/linux/lsm_hook_defs.h +++ b/include/linux/lsm_hook_defs.h @@ -71,6 +71,7 @@ LSM_HOOK(int, 0, sb_show_options, struct seq_file *m, struct super_block *sb) LSM_HOOK(int, 0, sb_statfs, struct dentry *dentry) LSM_HOOK(int, 0, sb_mount, const char *dev_name, const struct path *path, const char *type, unsigned long flags, void *data) +LSM_HOOK(int, 0, sb_bindmount, const struct path *old_path, const struct path *path) LSM_HOOK(int, 0, sb_umount, struct vfsmount *mnt, int flags) LSM_HOOK(int, 0, sb_pivotroot, const struct path *old_path, const struct path *new_path) diff --git a/include/linux/security.h b/include/linux/security.h index cbdba435b798..512ac656500e 100644 --- a/include/linux/security.h +++ b/include/linux/security.h @@ -365,6 +365,7 @@ int security_sb_show_options(struct seq_file *m, struct super_block *sb); int security_sb_statfs(struct dentry *dentry); int security_sb_mount(const char *dev_name, const struct path *path, const char *type, unsigned long flags, void *data); +int security_sb_bindmount(const struct path *old_path, const struct path *path); int security_sb_umount(struct vfsmount *mnt, int flags); int security_sb_pivotroot(const struct path *old_path, const struct path *new_path); int security_sb_set_mnt_opts(struct super_block *sb, diff --git a/security/security.c b/security/security.c index 09664e09fec9..bd7cb3df16f4 100644 --- a/security/security.c +++ b/security/security.c @@ -1564,6 +1564,22 @@ int security_sb_mount(const char *dev_name, const struct path *path, return call_int_hook(sb_mount, dev_name, path, type, flags, data); } +/** + * security_sb_bindmount() - Loopback/bind mount specific permission check + * @old_path: source of loopback/bind mount + * @path: mount point + * + * This check is performed in addition to security_sb_mount and only if the + * mount type is determined to be loopback/bind mount (flags & MS_BIND). It + * surfaces the mount source as a path struct. + * + * Return: Returns 0 if permission is granted. + */ +int security_sb_bindmount(const struct path *old_path, const struct path *path) +{ + return call_int_hook(sb_bindmount, old_path, path); +} + /** * security_sb_umount() - Check permission for unmounting a filesystem * @mnt: mounted filesystem
The main mount security hook (security_sb_mount) is called early in the process before the mount type is determined and the arguments are validated and converted to the appropriate format. Specifically, the source path is surfaced as a string, which is not appropriate for checking bind mount requests. For bind mounts the source should be validated and passed as a path struct (same as destination), after the mount type is determined. This allows the hook users to evaluate the mount attributes without the need to perform any validations or conversions out of band, which can introduce a TOCTOU race condition. The newly introduced hook is invoked only if the security_sb_mount hook passes, and only if the MS_BIND flag is detected. At this point the source of the mount has been successfully converted to a path struct using the kernel's kern_path API. This allows LSMs to target bind mount requests at the right stage, and evaluate the attributes in the right format, based on the type of mount. This does not affect the functionality of the existing mount security hooks, including security_sb_mount. The new hook, can be utilized as a supplement to the main hook for further analyzing bind mount requests. This means that there is still the option of only using the main hook function, if all one wants to do is indiscriminately reject all bind mount requests, regardless of the source and destination arguments. However, if one needs to evaluate the source and destination of a bind mount request before making a decision, this hook function should be preferred. Of course, if a bind mount request does not make it past the security_sb_mount check, the bind mount hook function is never invoked. Signed-off-by: Shervin Oloumi <enlightened@chromium.org> --- fs/namespace.c | 4 ++++ include/linux/lsm_hook_defs.h | 1 + include/linux/security.h | 1 + security/security.c | 16 ++++++++++++++++ 4 files changed, 22 insertions(+) base-commit: fc033cf25e612e840e545f8d5ad2edd6ba613ed5