Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] proc: unpinned dereference of pde->seq_ops->next in pde_seq_next() --
commit ae41131fe28a1e7fcf9c3b743e2a30697e315fe9 Author: Christian Brauner <[email protected]> proc: let procfs own the seq_file of seq_file entries This commit allows procfs to own the seq_file for seq_file and single-show entries by wrapping their seq_operations and pinning the proc_dir_entry only around start, next, show, and stop callbacks rather than across userspace copies in seq_read_iter(). > diff --git a/fs/proc/inode.c b/fs/proc/inode.c > index 66a3688757dd7..8425465aae5d7 100644 > --- a/fs/proc/inode.c > +++ b/fs/proc/inode.c > @@ -218,6 +218,110 @@ static void pde_unpin(struct proc_dir_entry *pde) > unuse_pde(pde); > } > [ ... ] > +static void pde_seq_stop_pinned(struct seq_file *m, void *v) > +{ > + struct proc_dir_entry *pde = seq_pde(m); > + > + if (pde_has_proc_seq_ops(pde)) > + pde->seq_ops->stop(m, v); > + pde_unpin(pde); > +} > + > +static void *pde_seq_start(struct seq_file *m, loff_t *pos) > +{ > + struct proc_dir_entry *pde = seq_pde(m); > + void *v; > + > + if (!pde_pin(pde)) > + return ERR_PTR(-EIO); > + if (!pde_has_proc_seq_ops(pde)) > + return single_start(m, pos); > + v = pde->seq_ops->start(m, pos); > + if (v == ERR_PTR(-EIO)) > + pde_seq_stop_pinned(m, v); > + return v; > +} > + > +static void *pde_seq_next(struct seq_file *m, void *v, loff_t *pos) > +{ > + struct proc_dir_entry *pde = seq_pde(m); > + loff_t old = *pos; > + > + if (!pde_has_proc_seq_ops(pde)) { > + ++*pos; > + return NULL; > + } > + v = pde->seq_ops->next(m, v, pos); > + if (v == ERR_PTR(-EIO)) > + pde_seq_stop_pinned(m, v); > + /* seq_file would blame these seq_operations, blame the entry's */ > + if (*pos == old) { > + pr_info_ratelimited("buggy .next function %ps did not update > position index\n", > + pde->seq_ops->next); > + ++*pos; > + } > + return v; > +} [Severity: High] In pde_seq_next(), if pde->seq_ops->next(m, v, pos) returns ERR_PTR(-EIO) without advancing *pos, pde_seq_stop_pinned(m, v) is called before the if (*pos == old) check. Since pde_seq_stop_pinned() calls pde_unpin(pde), which decrements pde->in_use and can complete pde->pde_unload_completion, a concurrent remove_proc_entry() waiting in proc_entry_rundown() can finish and allow a module to unload or free the memory backing pde->seq_ops before pr_info_ratelimited() dereferences pde->seq_ops->next and formats it with %ps. In addition, pde->seq_ops->stop(m, v) in pde_seq_stop_pinned() will observe the unincremented *pos value. Could the if (*pos == old) check be moved ahead of the if (v == ERR_PTR(-EIO)) check so that pde->seq_ops->next is only accessed while the entry is still pinned and *pos is updated before ->stop() is called? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=4
