On Mon, Dec 14, 2020 at 3:24 PM Miklos Szeredi <miklos@xxxxxxxxxx> wrote: > > On Mon, Dec 14, 2020 at 6:44 AM Amir Goldstein <amir73il@xxxxxxxxx> wrote: > > > Perhaps, but there is a much bigger issue with this change IMO. > > Not because of dropping rule (b) of the permission model, but because > > of relaxing rule (a). > > > > Should overlayfs respect the conservative interpretation as it partly did > > until this commit, a lower file must not lose IMMUTABLE/APPEND_ONLY > > after copy up, but that is exactly what is going to happen if we first > > copy up and then fail permission check on setting the flags. > > Yeah, it's a mess. This will hopefully sort it out, as it will allow > easier copy up of flags: > > https://lore.kernel.org/linux-fsdevel/20201123141207.GC327006@xxxxxxxxxxxxxxxxxxxxxxxxx/ > > In actual fact losing S_APPEND is not currently prevented by copy-up > triggered by anything other than FS_IOC_SETX*, and even that is prone > to races as indicated by the bug report that resulted in this patch. > > Let's just fix the IMMUTABLE case: > > - if the file is already copied up with data (since the overlay > ioctl implementation currently uses the realdata), then we're fine to > copy up > > - if the file is not IMMUTABLE to start with, then also fine to copy > up; even if the op will fail after copy up we haven't done anything > that wouldn't be possible without this particular codepath > > - if task has CAP_LINUX_IMMUTABLE (can add/remove immutable) then > it's also fine to copy up since we can be fairly sure that the actual > setflags will succeed as well. If not, that can be a problem, but as > I've said copying up IMMUTABLE and other flags should really be done > by the copy up code, otherwise it won't work properly. > > Something like this incremental should be good, I think: > > @@ -576,6 +576,15 @@ static long ovl_ioctl_set_flags(struct f > > inode_lock(inode); > > + /* > + * Prevent copy up if immutable and has no CAP_LINUX_IMMUTABLE > + * capability. > + */ > + ret = -EPERM; > + if (!ovl_has_upperdata(inode) && IS_IMMUTABLE(inode) && > + !capable(CAP_LINUX_IMMUTABLE)) > + goto unlock; > + > ret = ovl_maybe_copy_up(file_dentry(file), O_WRONLY); > if (ret) > goto unlock; > I guess that looks ok for a band aid. Thanks, Amir.
