On Mon 02-11-26 15:21:34, Christian Brauner wrote:
> proc_readdir_de() counts the entries of a directory and resumes a
> listing at the number getdents64() stopped at. The entries are sorted
> by name, so one that is added or removed in between shifts everything
> behind it by one. Listing /proc/net/dev_snmp6 with a buffer that holds
> ".", ".." and "lo" and adding a device whose name sorts before "lo"
> makes the next call return "lo" a second time. Removing one that sorted
> before the position makes the next call skip an entry instead.
>
> Since commit 895b4c0c79b0 ("fs/proc: fix uaf in proc_readdir_de()") an
> entry that is removed while it is being emitted also ends the listing
> right there. pde_subdir_next() finds nothing after an erased entry, so
> proc_readdir_de() reports the end of the directory and the next call
> resumes by number, skipping another entry. For /proc itself
> proc_root_readdir() takes that as the end of the static entries and
> moves on to the pids, so the rest of the static entries is missing.
> And if the position is behind the last entry when a call starts
> proc_readdir_de() returns 0, so /proc then lists no pids at all.
>
> So remember the name of the last entry a call emitted in the file and
> resume at the first entry after it, like kernfs resumes after the node
> it stopped at. An entry that was removed while it was emitted is
> continued after by its name as well. If the position was moved by a
> seek in between the listing counts from the start like before.
>
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Fixes: 895b4c0c79b0 ("fs/proc: fix uaf in proc_readdir_de()")
> Cc: [email protected]
> Signed-off-by: Christian Brauner (Amutable) <[email protected]>
Looks good. Feel free to add:
Reviewed-by: Jan Kara <[email protected]>
Honza
> ---
> fs/proc/generic.c | 89
> ++++++++++++++++++++++++++++++++++++++++++++++--------
> fs/proc/internal.h | 1 +
> fs/proc/proc_net.c | 1 +
> fs/proc/root.c | 1 +
> 4 files changed, 80 insertions(+), 12 deletions(-)
>
> diff --git a/fs/proc/generic.c b/fs/proc/generic.c
> index ef804cd3ec45..47830d6af255 100644
> --- a/fs/proc/generic.c
> +++ b/fs/proc/generic.c
> @@ -87,6 +87,28 @@ static struct proc_dir_entry *pde_subdir_find(struct
> proc_dir_entry *dir,
> return NULL;
> }
>
> +/* The first entry of @dir that sorts after @name. */
> +static struct proc_dir_entry *pde_subdir_after(struct proc_dir_entry *dir,
> + const char *name,
> + unsigned int len)
> +{
> + struct rb_node *node = dir->subdir.rb_node, *after = NULL;
> +
> + while (node) {
> + struct proc_dir_entry *de = rb_entry(node,
> + struct proc_dir_entry,
> + subdir_node);
> +
> + if (proc_match(name, de, len) < 0) {
> + after = node;
> + node = node->rb_left;
> + } else {
> + node = node->rb_right;
> + }
> + }
> + return rb_entry_safe(after, struct proc_dir_entry, subdir_node);
> +}
> +
> static bool pde_subdir_insert(struct proc_dir_entry *dir,
> struct proc_dir_entry *de)
> {
> @@ -277,6 +299,40 @@ struct dentry *proc_lookup(struct inode *dir, struct
> dentry *dentry,
> return proc_lookup_de(dir, dentry, PDE(dir));
> }
>
> +/*
> + * Where the next readdir resumes. The entries are counted, so one that is
> + * added or removed in front of the position would shift what the next call
> + * returns. The name of the last entry emitted doesn't move.
> + */
> +struct proc_dir_cursor {
> + loff_t pos; /* the position behind the entry */
> + unsigned int len;
> + char name[NAME_MAX + 1];
> +};
> +
> +/* If this can't remember the entry the next call counts from the start. */
> +static void proc_dir_cursor_set(struct file *file, struct proc_dir_entry *de,
> + loff_t pos)
> +{
> + struct proc_dir_cursor *cursor = file->private_data;
> +
> + if (!cursor) {
> + cursor = kmalloc(sizeof(*cursor), GFP_KERNEL);
> + if (!cursor)
> + return;
> + file->private_data = cursor;
> + }
> + cursor->pos = pos;
> + cursor->len = de->namelen;
> + memcpy(cursor->name, de->name, de->namelen);
> +}
> +
> +int proc_dir_release(struct inode *inode, struct file *file)
> +{
> + kfree(file->private_data);
> + return 0;
> +}
> +
> /*
> * This returns non-zero if at EOF, so that the /proc
> * root directory can use this and check if it should
> @@ -289,23 +345,26 @@ struct dentry *proc_lookup(struct inode *dir, struct
> dentry *dentry,
> int proc_readdir_de(struct file *file, struct dir_context *ctx,
> struct proc_dir_entry *de)
> {
> + struct proc_dir_cursor *cursor = file->private_data;
> + struct proc_dir_entry *dir = de;
> int i;
>
> if (!dir_emit_dots(file, ctx))
> return 0;
>
> - i = ctx->pos - 2;
> read_lock(&proc_subdir_lock);
> - de = pde_subdir_first(de);
> - for (;;) {
> - if (!de) {
> - read_unlock(&proc_subdir_lock);
> - return 0;
> - }
> - if (!i)
> - break;
> - de = pde_subdir_next(de);
> - i--;
> + if (cursor && cursor->pos == ctx->pos) {
> + /* Resume after the last entry emitted, wherever it is now. */
> + de = pde_subdir_after(dir, cursor->name, cursor->len);
> + } else {
> + /* Moved by a seek, count from the start like always. */
> + de = pde_subdir_first(dir);
> + for (i = ctx->pos - 2; de && i; i--)
> + de = pde_subdir_next(de);
> + }
> + if (!de) {
> + read_unlock(&proc_subdir_lock);
> + return 1;
> }
>
> do {
> @@ -318,8 +377,13 @@ int proc_readdir_de(struct file *file, struct
> dir_context *ctx,
> return 0;
> }
> ctx->pos++;
> + proc_dir_cursor_set(file, de, ctx->pos);
> read_lock(&proc_subdir_lock);
> - next = pde_subdir_next(de);
> + /* Removed in the meantime, go on after its name. */
> + if (RB_EMPTY_NODE(&de->subdir_node))
> + next = pde_subdir_after(dir, de->name, de->namelen);
> + else
> + next = pde_subdir_next(de);
> pde_put(de);
> de = next;
> } while (de);
> @@ -347,6 +411,7 @@ static const struct file_operations proc_dir_operations =
> {
> .llseek = generic_file_llseek,
> .read = generic_read_dir,
> .iterate_shared = proc_readdir,
> + .release = proc_dir_release,
> };
>
> static int proc_net_d_revalidate(struct inode *dir, const struct qstr *name,
> diff --git a/fs/proc/internal.h b/fs/proc/internal.h
> index df873df9afc5..04f7ce2b24ed 100644
> --- a/fs/proc/internal.h
> +++ b/fs/proc/internal.h
> @@ -287,6 +287,7 @@ extern struct dentry *proc_lookup(struct inode *, struct
> dentry *, unsigned int)
> struct dentry *proc_lookup_de(struct inode *, struct dentry *, struct
> proc_dir_entry *);
> extern int proc_readdir(struct file *, struct dir_context *);
> int proc_readdir_de(struct file *, struct dir_context *, struct
> proc_dir_entry *);
> +int proc_dir_release(struct inode *, struct file *);
>
> static inline void pde_get(struct proc_dir_entry *pde)
> {
> diff --git a/fs/proc/proc_net.c b/fs/proc/proc_net.c
> index b1f5eafb069a..dbcf87a257e1 100644
> --- a/fs/proc/proc_net.c
> +++ b/fs/proc/proc_net.c
> @@ -351,6 +351,7 @@ const struct file_operations proc_net_operations = {
> .llseek = generic_file_llseek,
> .read = generic_read_dir,
> .iterate_shared = proc_tgid_net_readdir,
> + .release = proc_dir_release,
> };
>
> static __net_init int proc_net_ns_init(struct net *net)
> diff --git a/fs/proc/root.c b/fs/proc/root.c
> index 7fbbe92bf73a..dfee1caa26f7 100644
> --- a/fs/proc/root.c
> +++ b/fs/proc/root.c
> @@ -441,6 +441,7 @@ static const struct file_operations proc_root_operations
> = {
> .read = generic_read_dir,
> .iterate_shared = proc_root_readdir,
> .llseek = generic_file_llseek,
> + .release = proc_dir_release,
> };
>
> /*
>
> --
> 2.53.0
>
--
Jan Kara <[email protected]>
SUSE Labs, CR
