Re: [RESEND PATCH] vfs: add RWF_NOAPPEND flag for pwritev2

On Mon, Aug 31, 2020 at 3:46 AM Rich Felker [off-list ref] wrote:

On Mon, Aug 31, 2020 at 03:15:04AM +0200, Jann Horn wrote:

quoted

On Sun, Aug 30, 2020 at 10:00 PM Rich Felker [off-list ref] wrote:

quoted

On Sun, Aug 30, 2020 at 09:02:31PM +0200, Jann Horn wrote:

quoted

On Sun, Aug 30, 2020 at 8:43 PM Rich Felker [off-list ref] wrote:

quoted

On Sun, Aug 30, 2020 at 08:31:36PM +0200, Jann Horn wrote:

quoted

On Sun, Aug 30, 2020 at 6:36 PM Rich Felker [off-list ref] wrote:

quoted

So just checking IS_APPEND in the code paths used by
pwritev2 (and erroring out rather than silently writing output at the
wrong place) should suffice to preserve all existing security
invariants.

Makes sense.

There are 3 places where kiocb_set_rw_flags is called with flags that
seem to be controlled by userspace: aio.c, io_uring.c, and
read_write.c. Presumably each needs to EPERM out on RWF_NOAPPEND if
the underlying inode is S_APPEND. To avoid repeating the same logic in
an error-prone way, should kiocb_set_rw_flags's signature be updated
to take the filp so that it can obtain the inode and check IS_APPEND
before accepting RWF_NOAPPEND? It's inline so this should avoid
actually loading anything except in the codepath where
flags&RWF_NOAPPEND is nonzero.

You can get the file pointer from ki->ki_filp. See the RWF_NOWAIT
branch of kiocb_set_rw_flags().

Thanks. I should have looked for that. OK, so a fixup like this on top
of the existing patch?

diff --git a/include/linux/fs.h b/include/linux/fs.h
index 473289bff4c6..674131e8d139 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h

@@ -3457,8 +3457,11 @@ static inline int kiocb_set_rw_flags(struct kiocb *ki, rwf_t flags)
                ki->ki_flags |= (IOCB_DSYNC | IOCB_SYNC);
        if (flags & RWF_APPEND)
                ki->ki_flags |= IOCB_APPEND;
-       if (flags & RWF_NOAPPEND)
+       if (flags & RWF_NOAPPEND) {
+               if (IS_APPEND(file_inode(ki->ki_filp)))
+                       return -EPERM;
                ki->ki_flags &= ~IOCB_APPEND;
+       }
        return 0;
 }

If this is good I'll submit a v2 as the above squashed with the
original patch.

Looks good to me.

Actually it's not quite. I think it should be:

        if ((flags & RWF_NOAPPEND) & (ki->ki_flags & IOCB_APPEND)) {
                if (IS_APPEND(file_inode(ki->ki_filp)))
                        return -EPERM;
                ki->ki_flags &= ~IOCB_APPEND;
        }

i.e. don't refuse RWF_NOAPPEND on a file that was already successfully
opened without O_APPEND that only subsequently got chattr +a. The
permission check should only be done if it's overriding the default
action for how the file is open.

This is actually related to the fcntl corner case mentioned before.

Are you ok with this change? If so I'll go ahead and prepare a v2.

Ah, yeah, I guess that makes sense to keep things more consistent.

(You'll have to write that as "(flags & RWF_NOAPPEND) && (ki->ki_flags
& IOCB_APPEND)" though (logical AND, not bitwise AND).)