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 Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 55E12C43217 for ; Thu, 10 Mar 2022 00:36:36 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S239001AbiCJAhe (ORCPT ); Wed, 9 Mar 2022 19:37:34 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:45630 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S233863AbiCJAhc (ORCPT ); Wed, 9 Mar 2022 19:37:32 -0500 Received: from smtp-relay-canonical-1.canonical.com (smtp-relay-canonical-1.canonical.com [185.125.188.121]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 269B61052A2; Wed, 9 Mar 2022 16:36:32 -0800 (PST) Received: from [192.168.192.153] (unknown [50.126.114.69]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by smtp-relay-canonical-1.canonical.com (Postfix) with ESMTPSA id 0F6043F12B; Thu, 10 Mar 2022 00:36:19 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=canonical.com; s=20210705; t=1646872582; bh=aGcEVKAOrTGJDxPnhMFB8dBVhKHNAxc+BYirL6/tWsM=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=BYcRRKMHEhVvCIE5GQndE0fWH8AB88n1S+NB+OFT5mhAdCfel9WaZnM7MMLoMwdJQ B2oh8wyzyiPADr5/tlD9kTKjsC+csb1cGCRZGjsAMms3Hx0NsCb9M+b0Mr2o0rMP1Y Zay8WaezyhcUvaucSu1O5f0KOzdBDM4lUG7wyEJfOmAi/n6j+lCUGSCNjzy8MMmu/9 SIfbic208WqVNTTRroZTJWwC4ZhfR6ZxY7RqdjIq/h6hqENjBzNTi5ttL2W4R7uNtV jSNaMsl9THVP1b6TK8KjBKMVLKrSbxV3OYaQ92a6MyWXj7zV1ctF8K2s4EhOObdF7C XrboieAjFlQRw== Message-ID: <8d520529-4d3e-4874-f359-0ead9207cead@canonical.com> Date: Wed, 9 Mar 2022 16:36:04 -0800 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.5.0 Subject: Re: [PATCH v1] fs: Fix inconsistent f_mode Content-Language: en-US To: Paul Moore , =?UTF-8?Q?Micka=c3=abl_Sala=c3=bcn?= , Al Viro Cc: Christian Brauner , "Darrick J . Wong" , Eric Paris , James Morris , Kentaro Takeda , Miklos Szeredi , "Serge E . Hallyn" , Stephen Smalley , Steve French , Tetsuo Handa , linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org, linux-security-module@vger.kernel.org, =?UTF-8?Q?Micka=c3=abl_Sala=c3=bcn?= , selinux@vger.kernel.org, Casey Schaufler References: <20220228215935.748017-1-mic@digikod.net> <20220301092232.wh7m3fxbe7hyxmcu@wittgenstein> From: John Johansen Organization: Canonical In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 3/9/22 13:31, Paul Moore wrote: > On Tue, Mar 1, 2022 at 5:15 AM Mickaël Salaün wrote: >> On 01/03/2022 10:22, Christian Brauner wrote: >>> On Mon, Feb 28, 2022 at 10:59:35PM +0100, Mickaël Salaün wrote: >>>> From: Mickaël Salaün >>>> >>>> While transitionning to ACC_MODE() with commit 5300990c0370 ("Sanitize >>>> f_flags helpers") and then fixing it with commit 6d125529c6cb ("Fix >>>> ACC_MODE() for real"), we lost an open flags consistency check. Opening >>>> a file with O_WRONLY | O_RDWR leads to an f_flags containing MAY_READ | >>>> MAY_WRITE (thanks to the ACC_MODE() helper) and an empty f_mode. >>>> Indeed, the OPEN_FMODE() helper transforms 3 (an incorrect value) to 0. >>>> >>>> Fortunately, vfs_read() and vfs_write() both check for FMODE_READ, or >>>> respectively FMODE_WRITE, and return an EBADF error if it is absent. >>>> Before commit 5300990c0370 ("Sanitize f_flags helpers"), opening a file >>>> with O_WRONLY | O_RDWR returned an EINVAL error. Let's restore this safe >>>> behavior. >>> >>> That specific part seems a bit risky at first glance. Given that the >>> patch referenced is from 2009 this means we've been allowing O_WRONLY | >>> O_RDWR to succeed for almost 13 years now. >> >> Yeah, it's an old bug, but we should keep in mind that a file descriptor >> created with such flags cannot be used to read nor write. However, >> unfortunately, it can be used for things like ioctl, fstat, chdir… I >> don't know if there is any user of this trick. >> >> Either way, there is an inconsistency between those using ACC_MODE() and >> those using OPEN_FMODE(). If we decide to take a side for the behavior >> of one or the other, without denying to create such FD, it could also >> break security policies. We have to choose what to potentially break… > > I'm not really liking the idea that the empty/0 f_mode field leads to > SELinux doing an ioctl access check as opposed to the expected > read|write check. Yes, other parts of the code catch the problem, but > this is bad from a SELinux perspective. Looking quickly at the other > LSMs, it would appear that other LSMs are affected as well. > > If we're not going to fix file::f_mode, the LSMs probably need to > consider using file::f_flags directly in conjunction with a correct > OPEN_FMODE() macro (or better yet a small inline function that isn't > as ugly). > yeah, I have to agree