From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-8.5 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id D3D59C433E7 for ; Mon, 31 Aug 2020 01:46:38 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id B9F0520782 for ; Mon, 31 Aug 2020 01:46:38 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727040AbgHaBqi (ORCPT ); Sun, 30 Aug 2020 21:46:38 -0400 Received: from brightrain.aerifal.cx ([216.12.86.13]:48326 "EHLO brightrain.aerifal.cx" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726618AbgHaBqf (ORCPT ); Sun, 30 Aug 2020 21:46:35 -0400 Date: Sun, 30 Aug 2020 21:46:33 -0400 From: Rich Felker To: Jann Horn Cc: linux-fsdevel , kernel list , Linux API , Alexander Viro Subject: Re: [RESEND PATCH] vfs: add RWF_NOAPPEND flag for pwritev2 Message-ID: <20200831014633.GJ3265@brightrain.aerifal.cx> References: <20200829020002.GC3265@brightrain.aerifal.cx> <20200830163657.GD3265@brightrain.aerifal.cx> <20200830184334.GE3265@brightrain.aerifal.cx> <20200830200029.GF3265@brightrain.aerifal.cx> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Aug 31, 2020 at 03:15:04AM +0200, Jann Horn wrote: > On Sun, Aug 30, 2020 at 10:00 PM Rich Felker wrote: > > On Sun, Aug 30, 2020 at 09:02:31PM +0200, Jann Horn wrote: > > > On Sun, Aug 30, 2020 at 8:43 PM Rich Felker wrote: > > > > On Sun, Aug 30, 2020 at 08:31:36PM +0200, Jann Horn wrote: > > > > > On Sun, Aug 30, 2020 at 6:36 PM Rich Felker wrote: > > > > > > 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. Rich