* [PATCH] proc: fix inode uid/gid writeback race
@ 2019-10-20 17:30 Alexey Dobriyan
2019-10-21 9:24 ` Marco Elver
0 siblings, 1 reply; 5+ messages in thread
From: Alexey Dobriyan @ 2019-10-20 17:30 UTC (permalink / raw)
To: akpm; +Cc: linux-kernel, linux-fsdevel, elver, viro
(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@syzkaller.appspotmail.com
Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
---
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);
+}
+
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);
/*
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] proc: fix inode uid/gid writeback race 2019-10-20 17:30 [PATCH] proc: fix inode uid/gid writeback race Alexey Dobriyan @ 2019-10-21 9:24 ` Marco Elver 2019-10-21 9:59 ` Marco Elver 2019-10-21 18:32 ` Alexey Dobriyan 0 siblings, 2 replies; 5+ messages in thread From: Marco Elver @ 2019-10-21 9:24 UTC (permalink / raw) To: Alexey Dobriyan; +Cc: Andrew Morton, LKML, linux-fsdevel, viro On Sun, 20 Oct 2019 at 19:30, Alexey Dobriyan <adobriyan@gmail.com> 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@syzkaller.appspotmail.com > Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com> > --- 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@google.com> 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@google.com/ 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. > +} > + > 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); > /* ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] proc: fix inode uid/gid writeback race 2019-10-21 9:24 ` Marco Elver @ 2019-10-21 9:59 ` Marco Elver 2019-10-21 18:32 ` Alexey Dobriyan 1 sibling, 0 replies; 5+ messages in thread From: Marco Elver @ 2019-10-21 9:59 UTC (permalink / raw) To: Alexey Dobriyan; +Cc: Andrew Morton, LKML, linux-fsdevel, viro On Mon, 21 Oct 2019 at 11:24, Marco Elver <elver@google.com> wrote: > > On Sun, 20 Oct 2019 at 19:30, Alexey Dobriyan <adobriyan@gmail.com> 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@syzkaller.appspotmail.com > > Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com> > > --- > > 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@google.com> > > 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@google.com/ > 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); > > /* ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] proc: fix inode uid/gid writeback race 2019-10-21 9:24 ` Marco Elver 2019-10-21 9:59 ` Marco Elver @ 2019-10-21 18:32 ` Alexey Dobriyan 2019-11-03 13:20 ` Dmitry Vyukov 1 sibling, 1 reply; 5+ messages in thread From: Alexey Dobriyan @ 2019-10-21 18:32 UTC (permalink / raw) To: Marco Elver, dhowells; +Cc: Andrew Morton, LKML, linux-fsdevel, viro On Mon, Oct 21, 2019 at 11:24:27AM +0200, Marco Elver wrote: > On Sun, 20 Oct 2019 at 19:30, Alexey Dobriyan <adobriyan@gmail.com> 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@syzkaller.appspotmail.com > > Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com> > > --- > > 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@google.com> > > 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@google.com/ > 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. This probably requires bloating "struct cred" with generation number. I'm not sure what to do other than cc our credential overlords. > > +} > > + > > 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); > > /* ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] proc: fix inode uid/gid writeback race 2019-10-21 18:32 ` Alexey Dobriyan @ 2019-11-03 13:20 ` Dmitry Vyukov 0 siblings, 0 replies; 5+ messages in thread From: Dmitry Vyukov @ 2019-11-03 13:20 UTC (permalink / raw) To: Alexey Dobriyan Cc: Marco Elver, David Howells, Andrew Morton, LKML, linux-fsdevel, Al Viro On Mon, Oct 21, 2019 at 8:32 PM Alexey Dobriyan <adobriyan@gmail.com> wrote: > > On Mon, Oct 21, 2019 at 11:24:27AM +0200, Marco Elver wrote: > > On Sun, 20 Oct 2019 at 19:30, Alexey Dobriyan <adobriyan@gmail.com> 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@syzkaller.appspotmail.com > > > Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com> > > > --- > > > > 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@google.com> > > > > 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@google.com/ > > 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. > > This probably requires bloating "struct cred" with generation number. > I'm not sure what to do other than cc our credential overlords. Just in case, this bug can lead to bad observable behavior, e.g. bypassing LSM checks (apparmor) and this patch does not fix the underlying problem. For details see: https://groups.google.com/d/msg/syzkaller-bugs/mzwiXt4ml68/GuAUQrWtBQAJ > > > +} > > > + > > > 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); > > > /* ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-11-03 13:20 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-10-20 17:30 [PATCH] proc: fix inode uid/gid writeback race Alexey Dobriyan 2019-10-21 9:24 ` Marco Elver 2019-10-21 9:59 ` Marco Elver 2019-10-21 18:32 ` Alexey Dobriyan 2019-11-03 13:20 ` Dmitry Vyukov
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).