On Mon, 21 Oct 2019 at 11:24, Marco Elver <elver@xxxxxxxxxx> wrote:
>
> On Sun, 20 Oct 2019 at 19:30, Alexey Dobriyan <adobriyan@xxxxxxxxx> wrote:
> >
> > (euid, egid) pair is snapshotted correctly from task under RCU,
> > but writeback to inode can be done in any order.
> >
> > Fix by doing writeback under inode->i_lock where necessary
> > (/proc/* , /proc/*/fd/* , /proc/*/map_files/* revalidate).
> >
> > Reported-by: syzbot+e392f8008a294fdf8891@xxxxxxxxxxxxxxxxxxxxxxxxx
> > Signed-off-by: Alexey Dobriyan <adobriyan@xxxxxxxxx>
> > ---
>
> Thanks!
>
> This certainly fixes the problem of inconsistent uid/gid pair due to
> racing writebacks, as well as the data-race. If that is the only
> purpose of this patch, then from what I see this is fine:
>
> Acked-by: Marco Elver <elver@xxxxxxxxxx>
>
> However, there is probably still a more fundamental problem as outlined below.
>
> > fs/proc/base.c | 25 +++++++++++++++++++++++--
> > fs/proc/fd.c | 2 +-
> > fs/proc/internal.h | 2 ++
> > 3 files changed, 26 insertions(+), 3 deletions(-)
> >
> > --- a/fs/proc/base.c
> > +++ b/fs/proc/base.c
> > @@ -1743,6 +1743,25 @@ void task_dump_owner(struct task_struct *task, umode_t mode,
> > *rgid = gid;
> > }
> >
> > +/* use if inode is live */
> > +void task_dump_owner_to_inode(struct task_struct *task, umode_t mode,
> > + struct inode *inode)
> > +{
> > + kuid_t uid;
> > + kgid_t gid;
> > +
> > + task_dump_owner(task, mode, &uid, &gid);
> > + /*
> > + * There is no atomic "change all credentials at once" system call,
> > + * guaranteeing more than _some_ snapshot from "struct cred" ends up
> > + * in inode is not possible.
> > + */
> > + spin_lock(&inode->i_lock);
> > + inode->i_uid = uid;
> > + inode->i_gid = gid;
> > + spin_unlock(&inode->i_lock);
>
> 2 tasks can still race here, and the inconsistent scenario I outlined in
> https://lore.kernel.org/linux-fsdevel/000000000000328b2905951a7667@xxxxxxxxxx/
> could still happen I think (although extremely unlikely). Mainly,
> causality may still be violated -- but I may be wrong as I don't know
> the rest of the code (so please be critical of my suggestion).
>
> The problem is that if 2 threads race here, one has snapshotted old
> uid/gid, and the other the new uid/gid. Then it is still possible for
> the old uid/gid to be written back after new uid/gid, which would
> result in this bad scenario:
>
> === TASK 1 ===
> | seteuid(1000);
> | seteuid(0);
> | stat("/proc/<pid-of-task-1>", &fstat);
> | assert(fstat.st_uid == 0); // fails
> === TASK 2 ===
> | stat("/proc/<pid-of-task-1>", ...);
>
> AFAIK it's not something that can easily be fixed without some
> timestamp on the uid/gid pair (timestamp updated after setuid/seteuid
> etc) obtained in the RCU reader critical section. Then in this
> critical section, uid/gid should only be written if the current pair
> in inode is older according to snapshot timestamp.
Note that timestamp is probably wrong here, but rather some kind of
sequence number would be needed.
> > +}
> > +
> > struct inode *proc_pid_make_inode(struct super_block * sb,
> > struct task_struct *task, umode_t mode)
> > {
> > @@ -1769,6 +1788,7 @@ struct inode *proc_pid_make_inode(struct super_block * sb,
> > if (!ei->pid)
> > goto out_unlock;
> >
> > + /* fresh inode -- no races */
> > task_dump_owner(task, 0, &inode->i_uid, &inode->i_gid);
> > security_task_to_inode(task, inode);
> >
> > @@ -1802,6 +1822,7 @@ int pid_getattr(const struct path *path, struct kstat *stat,
> > */
> > return -ENOENT;
> > }
> > + /* "struct kstat" is thread local, atomic snapshot is enough */
> > task_dump_owner(task, inode->i_mode, &stat->uid, &stat->gid);
> > }
> > rcu_read_unlock();
> > @@ -1815,7 +1836,7 @@ int pid_getattr(const struct path *path, struct kstat *stat,
> > */
> > void pid_update_inode(struct task_struct *task, struct inode *inode)
> > {
> > - task_dump_owner(task, inode->i_mode, &inode->i_uid, &inode->i_gid);
> > + task_dump_owner_to_inode(task, inode->i_mode, inode);
> >
> > inode->i_mode &= ~(S_ISUID | S_ISGID);
> > security_task_to_inode(task, inode);
> > @@ -1990,7 +2011,7 @@ static int map_files_d_revalidate(struct dentry *dentry, unsigned int flags)
> > mmput(mm);
> >
> > if (exact_vma_exists) {
> > - task_dump_owner(task, 0, &inode->i_uid, &inode->i_gid);
> > + task_dump_owner_to_inode(task, 0, inode);
> >
> > security_task_to_inode(task, inode);
> > status = 1;
> > --- a/fs/proc/fd.c
> > +++ b/fs/proc/fd.c
> > @@ -101,7 +101,7 @@ static bool tid_fd_mode(struct task_struct *task, unsigned fd, fmode_t *mode)
> > static void tid_fd_update_inode(struct task_struct *task, struct inode *inode,
> > fmode_t f_mode)
> > {
> > - task_dump_owner(task, 0, &inode->i_uid, &inode->i_gid);
> > + task_dump_owner_to_inode(task, 0, inode);
> >
> > if (S_ISLNK(inode->i_mode)) {
> > unsigned i_mode = S_IFLNK;
> > --- a/fs/proc/internal.h
> > +++ b/fs/proc/internal.h
> > @@ -123,6 +123,8 @@ static inline struct task_struct *get_proc_task(const struct inode *inode)
> >
> > void task_dump_owner(struct task_struct *task, umode_t mode,
> > kuid_t *ruid, kgid_t *rgid);
> > +void task_dump_owner_to_inode(struct task_struct *task, umode_t mode,
> > + struct inode *inode);
> >
> > unsigned name_to_int(const struct qstr *qstr);
> > /*
