On Mon 02-11-26 15:21:36, Christian Brauner wrote:
> proc_remove() looks the entry up by its name in its parent and removes
> whatever it finds there:
>
> remove_proc_subtree(de->name, de->parent);
>
> If the entry was removed already, say along with its parent, that is
> either nothing or a different entry that got the same name in the
> meantime, and de->parent may well be gone. Two removals of the same
> entry are a bug in the caller, but going by the name turns it into
> removing an entry that belongs to somebody else.
>
> Since commit 895b4c0c79b0 ("fs/proc: fix uaf in proc_readdir_de()")
> an entry that was unlinked has an empty rb node. So check for that
> under proc_subdir_lock and otherwise remove the entry itself, with the
> loop of remove_proc_subtree() moved into a helper both of them use.
> Nothing can help a caller that passes an entry that has been freed
> already.
>
> Reported-by: Chris Mason <[email protected]>
> Fixes: a8ca16ea7b0a ("proc: Supply a function to remove a proc entry by PDE")
> Signed-off-by: Christian Brauner (Amutable) <[email protected]>
One nitpick below. Otherwise feel free to add:
Reviewed-by: Jan Kara <[email protected]>
> -int remove_proc_subtree(const char *name, struct proc_dir_entry *parent)
> +/*
> + * Remove @root, already erased from its parent, and everything below it.
> + * Called with proc_subdir_lock held for writing, returns with it dropped.
> + */
> +static int __remove_proc_subtree(struct proc_dir_entry *root)
> {
> - struct proc_dir_entry *root = NULL, *de, *next;
> - const char *fn = name;
> - unsigned int len;
> -
> - write_lock(&proc_subdir_lock);
> - if (__xlate_proc_name(name, &parent, &fn) != 0) {
> - write_unlock(&proc_subdir_lock);
> - return -ENOENT;
> - }
> - len = strlen(fn);
> + struct proc_dir_entry *de = root, *next;
>
> - root = pde_subdir_find(parent, fn, len);
> - if (!root) {
> - write_unlock(&proc_subdir_lock);
> - return -ENOENT;
> - }
> - if (unlikely(pde_is_permanent(root))) {
> - write_unlock(&proc_subdir_lock);
> - WARN(1, "removing permanent /proc entry '%s/%s'",
> - root->parent->name, root->name);
> - return -EINVAL;
> - }
> - pde_erase(root, parent);
> -
> - de = root;
> while (1) {
> next = pde_subdir_first(de);
> if (next) {
> @@ -871,6 +852,34 @@ int remove_proc_subtree(const char *name, struct
> proc_dir_entry *parent)
> pde_put(root);
> return 0;
> }
> +
> +int remove_proc_subtree(const char *name, struct proc_dir_entry *parent)
> +{
> + struct proc_dir_entry *root = NULL;
> + const char *fn = name;
> + unsigned int len;
> +
> + write_lock(&proc_subdir_lock);
> + if (__xlate_proc_name(name, &parent, &fn) != 0) {
> + write_unlock(&proc_subdir_lock);
> + return -ENOENT;
> + }
> + len = strlen(fn);
> +
> + root = pde_subdir_find(parent, fn, len);
> + if (!root) {
> + write_unlock(&proc_subdir_lock);
> + return -ENOENT;
> + }
> + if (unlikely(pde_is_permanent(root))) {
> + write_unlock(&proc_subdir_lock);
> + WARN(1, "removing permanent /proc entry '%s/%s'",
> + root->parent->name, root->name);
> + return -EINVAL;
> + }
> + pde_erase(root, parent);
> + return __remove_proc_subtree(root);
> +}
> EXPORT_SYMBOL(remove_proc_subtree);
>
> void *proc_get_parent_data(const struct inode *inode)
> @@ -882,8 +891,23 @@ EXPORT_SYMBOL_GPL(proc_get_parent_data);
>
> void proc_remove(struct proc_dir_entry *de)
> {
> - if (de)
> - remove_proc_subtree(de->name, de->parent);
> + if (!de)
> + return;
> +
> + write_lock(&proc_subdir_lock);
> + /* Removed already, maybe along with its parent. */
> + if (RB_EMPTY_NODE(&de->subdir_node)) {
> + write_unlock(&proc_subdir_lock);
> + return;
> + }
> + if (unlikely(pde_is_permanent(de))) {
> + write_unlock(&proc_subdir_lock);
> + WARN(1, "removing permanent /proc entry '%s/%s'",
> + de->parent->name, de->name);
> + return;
> + }
> + pde_erase(de, de->parent);
> + __remove_proc_subtree(de);
I'd move the RB_EMPTY_NODE() and pde_is_permanent() along with pde_erase()
call back to __remove_proc_subtree(). Both callers need them (so it is
unnecessary duplication) and also logically it makes sense there.
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR
