Re: [PATCH] cred: Propagate security_prepare_creds() error code
From: Amir Goldstein <amir73il@gmail.com>
Date: 2022-05-24 04:44:58
Also in:
keyrings, linux-cifs, linux-fsdevel, linux-mm, linux-nfs, linux-unionfs, lkml, netdev, selinux
On Sat, May 21, 2022 at 2:17 PM Frederick Lawler [off-list ref] 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 <redacted> ---
[...]
quoted hunk ↗ jump to hunk
@@ -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
quoted hunk ↗ jump to hunk
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.
quoted hunk ↗ jump to hunk
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.