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