NAK: [PATCH][SRU][Disco] UBUNTU: SAUCE: shiftfs: lock down certain superblock flags

[Christian Brauner]

On Tue, May 07, 2019 at 02:50:27PM -0500, Seth Forshee wrote:
> On Thu, May 02, 2019 at 11:47:02PM +0200, Christian Brauner wrote:
> > +static void shiftfs_super_force_flags(struct super_block *sb,
> > +				      unsigned long lower_flags)
> > +{
> > +	if (lower_flags & SB_RDONLY)
> > +		sb->s_flags |= SB_RDONLY;
> > +
> > +	if (lower_flags & SB_NOSUID)
> > +		sb->s_flags |= SB_NOSUID;
> > +
> > +	if (lower_flags & SB_NODEV)
> > +		sb->s_flags |= SB_NODEV;
> > +
> > +	if (lower_flags & SB_NOEXEC)
> > +		sb->s_flags |= SB_NOEXEC;
> > +
> > +	if (lower_flags & SB_NOATIME)
> > +		sb->s_flags |= SB_NOATIME;
> > +
> > +	if (lower_flags & SB_NODIRATIME)
> > +		sb->s_flags |= SB_NODIRATIME;
> 
> It seems kind of odd to silently set rather important flags that weren't
> requested rather than returning an error, though I do find precedent for
> it. Seems like unexpectedly ending up with a read-only or noexec
> filesystem isn't great, but I guess it's a thing the kernel does.

Yep, it's the standard right now. Not sure, what the new mount api will
end up doing though...

> 
> But what about making this more succinct, something like:
> 
>         sb->s_flags |= lower_flags & (SB_RDONLY | SB_NOSUID | SB_NODEV |
>                                       SB_NOEXEC | SB_NOATIME | SB_NODIRATIME);

Yep, sounds good, will do.

> 
> > +
> > +	if (!(lower_flags & SB_POSIXACL))
> > +		sb->s_flags &= ~SB_POSIXACL;
> > +}
> > +
> >  static int shiftfs_fill_super(struct super_block *sb, void *raw_data,
> >  			      int silent)
> >  {
> > @@ -1904,6 +1960,8 @@ static int shiftfs_fill_super(struct super_block *sb, void *raw_data,
> >  		 */
> >  		sb->s_iflags = SB_I_NOEXEC;
> >  
> > +		shiftfs_super_force_flags(sb, lower_sb->s_flags);
> > +
> >  		/*
> >  		 * Handle nesting of shiftfs mounts by referring this mark
> >  		 * mount back to the original mark mount. This is more
> > @@ -1972,6 +2030,7 @@ static int shiftfs_fill_super(struct super_block *sb, void *raw_data,
> >  		 * passthrough settings.
> >  		 */
> >  		sbinfo->passthrough_mark = sbinfo_mp->passthrough;
> > +		shiftfs_super_force_flags(sb, path.mnt->mnt_sb->s_flags);
> >  	}
> 
> You're calling this before shiftfs_fill_super() unconditionally sets
> SB_POSIXACL, which renders forcing that flag ineffective. This needs to
> be fixed.

Oh good catch!
I think I'll just move setting that flag further up.

Thanks!
Christian