On Sat, May 20, 2023 at 06:55:44PM +0300, Amir Goldstein wrote:
> On Sat, May 20, 2023 at 4:19 PM Andrea Righi <andrea.righi@xxxxxxxxxxxxx> wrote:
> >
> > On Sat, May 20, 2023 at 03:33:32PM +0300, Amir Goldstein wrote:
> > > On Sat, May 20, 2023 at 3:20 PM Andrea Righi <andrea.righi@xxxxxxxxxxxxx> wrote:
> > ...
> > > > @@ -97,6 +99,8 @@ static inline struct mnt_idmap *ovl_upper_mnt_idmap(struct ovl_fs *ofs)
> > > >
> > > > static inline struct ovl_fs *OVL_FS(struct super_block *sb)
> > > > {
> > > > + /* Make sure OVL_FS() is always used with an overlayfs superblock */
> > > > + BUG_ON(sb->s_magic != OVERLAYFS_SUPER_MAGIC);
> > >
> > > 1. Adding new BUG_ON to kernel code is not acceptable - if anything
> > > you can add WARN_ON_ONCE()
> >
> > OK, but accessing a pointer to a struct ovl_fs that is not really a
> > struct ovl_fs can potentially have nasty effects, even data corruption
> > maybe? I'd rather crash the system now rather than experiencing random
> > behaviors later...
> >
>
> What you would rather do does not matter here.
> No new BUG_ON() is a rule set by Linus.
> Yes, some people (security people mostly) will prefer to crash the system
> over an "undefined" behavior later, but many non-security people would
> much rather have some processes stuck or crash than losing access to
> the entire system.
> There is no one good answer, but it is Linus who gets to decide.
I see, WARN_ON_ONCE() then seems more appropriate.
>
> > > 2. If anything, you should check s_type == s_ovl_fs_type, not s_magic
> >
> > Hm.. is there a fast way to determine when sb->s_type == overlayfs?
> > Using get_fs_type() here seems quite expensive and I'm not even sure if
> > it's doable, is there a better way that I don't see?
> >
>
> Not sure what you mean.
> This is what I meant:
>
> +extern struct file_system_type ovl_fs_type;
> +
> static inline struct ovl_fs *OVL_FS(struct super_block *sb)
> {
> + WARN_ON_ONCE(sb->s_type != &ovl_fs_type);
> +
> return (struct ovl_fs *)sb->s_fs_info;
> }
>
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index f97ad8b40dbb..0c1f9d1e9135 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -2083,7 +2083,7 @@ static struct dentry *ovl_mount(struct
> file_system_type *fs_type, int flags,
> return mount_nodev(fs_type, flags, raw_data, ovl_fill_super);
> }
>
> -static struct file_system_type ovl_fs_type = {
> +struct file_system_type ovl_fs_type = {
> .owner = THIS_MODULE,
> .name = "overlay",
Ah yes, we need to make ovl_fs_type non static. If it's acceptable, that
would work.
>
> > > 3. It is very unclear to me that this check has that much value and OVL_FS()
> > > macro is very commonly used inside internal helpers, so please add a
> > > "why" to your patch - why do you think that it is desired and/or valuable
> > > to fortify OVL_FS() like this?
> >
> > Sure, I can send a v2 explaining why I think this is needed. Basically I
> > was debugging a custom overlayfs patch and after a while I realized that
> > I was accessing the sb->s_fs_info of a real path (not an overlayfs sb),
> > using OVL_FS() with a proper check would have saved a me a bunch of
> > time.
> >
>
> I think if you add this extra pedantic check, it should be ifdefed with
> some Kconfig for extra debugging.
>
> Maybe you could add CONFIG_OVERLAY_FS_DEBUG like some
> other fs have. I am not sure if fortifying OVL_FS() is worth it, but
> maybe this config will gain more pedantics checks in the future.
>
> It's really up to Miklos to decide if this is interesting for overlayfs.
I like the CONFIG_OVERLAY_FS_DEBUG idea. I'll send a v2 with these
changes, let's see if there's some interest in it. Thank you so much for
your suggestions!
-Andrea
