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 kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id DE24CC433F5 for ; Tue, 24 May 2022 04:44:54 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 5C6E96B0072; Tue, 24 May 2022 00:44:54 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 50C6B6B0073; Tue, 24 May 2022 00:44:54 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 3AC466B0072; Tue, 24 May 2022 00:44:54 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0014.hostedemail.com [216.40.44.14]) by kanga.kvack.org (Postfix) with ESMTP id 132456B0073 for ; Tue, 24 May 2022 00:44:54 -0400 (EDT) Received: from smtpin11.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay02.hostedemail.com (Postfix) with ESMTP id C6BF5327EF for ; Tue, 24 May 2022 04:44:53 +0000 (UTC) X-FDA: 79499396466.11.66ED67A Received: from mail-qk1-f170.google.com (mail-qk1-f170.google.com [209.85.222.170]) by imf03.hostedemail.com (Postfix) with ESMTP id 05D2020020; Tue, 24 May 2022 04:44:40 +0000 (UTC) Received: by mail-qk1-f170.google.com with SMTP id 135so11797290qkm.4; Mon, 23 May 2022 21:44:52 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=3ji4sol5sENBEUR3FRJaaPGspSAIjqZFEyUSpEFLQDI=; b=hYQ0c6yYUcVfmu71erT+6VDvx5G5eYcf4JX8bivgxAcVRq5T3YZxHLiHp7WVwTBBKX 2cOVjoP55GH4W2zie2kWBOVkgG9jKEKwEHZKlBMLHQoU3Yc8QfBpCdlzyGoKWq6exl2z CXs+AIZOVs51bvpDx4A3SsKyoRq8+ZebiCv1qR8S+d2mqUxAJ/h6mxa3tZBaewM+RluK 1wzAbw2iLe8Oy3Zk36z2kqKEuVS4oPAbyUAezph7jfU7+2GCbslF5ktHoXqlE7+7ecDQ 5aw2ZDoJlyjN0AESyO2s/eOFveXNANFGXQV6JyxvQmmQbEnlvdWqWNb53GgzP2Uj38Kk pnCQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=3ji4sol5sENBEUR3FRJaaPGspSAIjqZFEyUSpEFLQDI=; b=HHBQMB0+dBlR9nPfgyf7DuVBkO9c1ItLyxfPkxSPyT5zmtuyUHdnrrAfJVCB/hbAdf nmeJUVw5bfSQXoEMLmvXdvWXUWxmncsmBi0V8r0PiqRK/Pk4m9BrqTXol+jsIktsdWD8 tIQjxLy34zvkNd0e2xAX86iAFRTAPWZs+rriKfEda2I6sXoV77fnm5UmgN++xPhps+g2 wbjTL4TE3bQct63hU/bTluCfErcdgNiRmIlGj657zVcgf7ulT+B3jv8k4KMNPyvmB7VT jWa2UJwaN2PUfT00T5ChrtWp0GoycwppVOe0zraeOzp347pQg9j/IVtseZfQTh3DZZM2 rvFA== X-Gm-Message-State: AOAM5329t7pz7JPtTZH3vHaNCWh20aQ/WXVbwGRVEn3NJ7cjfYh/ZID7 x0p4f2H+ItHlkMlFknAdgfySh16/03BrT6Znq/I= X-Google-Smtp-Source: ABdhPJyrMD7rEPjUOmV37i177VE1l/2iEx6x8NhX1oEETIuR1mmo3FjJT7Pvls+DCaJgQuyMwtU5RxBp/cx5GfJd574= X-Received: by 2002:a37:8846:0:b0:6a0:f6f1:a015 with SMTP id k67-20020a378846000000b006a0f6f1a015mr16143176qkd.386.1653367492354; Mon, 23 May 2022 21:44:52 -0700 (PDT) MIME-Version: 1.0 References: <20220520212746.95075-1-fred@cloudflare.com> In-Reply-To: <20220520212746.95075-1-fred@cloudflare.com> From: Amir Goldstein Date: Tue, 24 May 2022 07:44:41 +0300 Message-ID: Subject: Re: [PATCH] cred: Propagate security_prepare_creds() error code To: Frederick Lawler Cc: linux-aio@kvack.org, linux-fsdevel , linux-kernel , linux-cachefs@redhat.com, CIFS , samba-technical , Linux MM , Linux NFS Mailing List , overlayfs , LSM List , Netdev , keyrings@vger.kernel.org, selinux@vger.kernel.org, kernel-team@cloudflare.com Content-Type: text/plain; charset="UTF-8" X-Rspamd-Queue-Id: 05D2020020 X-Stat-Signature: tfxk5sjjb7udqc44kzzqdqwhtxhzo751 Authentication-Results: imf03.hostedemail.com; dkim=pass header.d=gmail.com header.s=20210112 header.b=hYQ0c6yY; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf03.hostedemail.com: domain of amir73il@gmail.com designates 209.85.222.170 as permitted sender) smtp.mailfrom=amir73il@gmail.com X-Rspam-User: X-Rspamd-Server: rspam06 X-HE-Tag: 1653367480-212420 X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Sat, May 21, 2022 at 2:17 PM Frederick Lawler wrote: > > While experimenting with the security_prepare_creds() LSM hook, we > noticed that our EPERM error code was not propagated up the callstack. > Instead ENOMEM is always returned. As a result, some tools may send a > confusing error message to the user: > > $ unshare -rU > unshare: unshare failed: Cannot allocate memory > > A user would think that the system didn't have enough memory, when > instead the action was denied. > > This problem occurs because prepare_creds() and prepare_kernel_cred() > return NULL when security_prepare_creds() returns an error code. Later, > functions calling prepare_creds() and prepare_kernel_cred() return > ENOMEM because they assume that a NULL meant there was no memory > allocated. > > Fix this by propagating an error code from security_prepare_creds() up > the callstack. > > Signed-off-by: Frederick Lawler > --- [...] > @@ -1173,7 +1173,7 @@ struct file *filp_open(const char *filename, int flags, umode_t mode) > { > struct filename *name = getname_kernel(filename); > struct file *file = ERR_CAST(name); > - > + stray whitespace > if (!IS_ERR(name)) { > file = file_open_name(name, flags, mode); > putname(name); > diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c > index f18490813170..905eb8f69d64 100644 > --- a/fs/overlayfs/dir.c > +++ b/fs/overlayfs/dir.c > @@ -589,28 +589,32 @@ static int ovl_create_or_link(struct dentry *dentry, struct inode *inode, > goto out_revert_creds; > } > > - err = -ENOMEM; > override_cred = prepare_creds(); > - if (override_cred) { > - override_cred->fsuid = inode->i_uid; > - override_cred->fsgid = inode->i_gid; > - if (!attr->hardlink) { > - err = security_dentry_create_files_as(dentry, > - attr->mode, &dentry->d_name, old_cred, > - override_cred); > - if (err) { > - put_cred(override_cred); > - goto out_revert_creds; > - } > - } > - put_cred(override_creds(override_cred)); > - put_cred(override_cred); > + if (IS_ERR(override_cred)) { > + err = PTR_ERR(override_cred); > + goto out_revert_creds; > + } > > - if (!ovl_dentry_is_whiteout(dentry)) > - err = ovl_create_upper(dentry, inode, attr); > - else > - err = ovl_create_over_whiteout(dentry, inode, attr); > + override_cred->fsuid = inode->i_uid; > + override_cred->fsgid = inode->i_gid; > + if (!attr->hardlink) { > + err = security_dentry_create_files_as(dentry, attr->mode, > + &dentry->d_name, > + old_cred, > + override_cred); > + if (err) { > + put_cred(override_cred); > + goto out_revert_creds; > + } > } > + put_cred(override_creds(override_cred)); > + put_cred(override_cred); > + > + if (!ovl_dentry_is_whiteout(dentry)) > + err = ovl_create_upper(dentry, inode, attr); > + else > + err = ovl_create_over_whiteout(dentry, inode, attr); > + It does not look like reducing the nesting level was really needed for your change. Was it? It is impossible to review a logic change with this much code churn. Please stick to the changes you declared you are doing and leave code style out of it. > out_revert_creds: > revert_creds(old_cred); > return err; > diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c > index 001cdbb8f015..b29b62670e10 100644 > --- a/fs/overlayfs/super.c > +++ b/fs/overlayfs/super.c > @@ -1984,10 +1984,11 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent) > if (!ofs) > goto out; > > - err = -ENOMEM; > ofs->creator_cred = cred = prepare_creds(); > - if (!cred) > + if (IS_ERR(ofs->creator_cred)) { > + err = PTR_ERR(ofs->creator_cred); > goto out_err; > + } > A non NULL must not be assigned to ofs->creator_cred use the cred local var for that check, otherwise things will go badly in ovl_free_fs(). Thanks, Amir.