Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220520212746.95075-1-fred@cloudflare.com>
In-Reply-To: <20220520212746.95075-1-fred@cloudflare.com>
From:   Amir Goldstein <amir73il@gmail.com>
Date:   Tue, 24 May 2022 07:44:41 +0300
Message-ID: <CAOQ4uxhOZu06xOHbwWszkvprjMVj4ZYbQMpdig0R1WH-4zZGCA@mail.gmail.com>
Subject: Re: [PATCH] cred: Propagate security_prepare_creds() error code
To:     Frederick Lawler <fred@cloudflare.com>
Cc:     linux-aio@kvack.org, linux-fsdevel <linux-fsdevel@vger.kernel.org>,
        linux-kernel <linux-kernel@vger.kernel.org>,
        linux-cachefs@redhat.com, CIFS <linux-cifs@vger.kernel.org>,
        samba-technical <samba-technical@lists.samba.org>,
        Linux MM <linux-mm@kvack.org>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>,
        overlayfs <linux-unionfs@vger.kernel.org>,
        LSM List <linux-security-module@vger.kernel.org>,
        Netdev <netdev@vger.kernel.org>, keyrings@vger.kernel.org,
        selinux@vger.kernel.org, kernel-team@cloudflare.com
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Sat, May 21, 2022 at 2:17 PM Frederick Lawler <fred@cloudflare.com> 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 <fred@cloudflare.com>
> ---
[...]
> @@ -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.