| Message ID | 20160803180432.1341-3-dh.herrmann@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Aug 03, 2016 at 08:04:26PM +0200, David Herrmann wrote: > diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c > index 323c238..e9d66f5 100644 > --- a/drivers/gpu/drm/drm_fops.c > +++ b/drivers/gpu/drm/drm_fops.c > @@ -199,7 +199,6 @@ static int drm_open_helper(struct file *filp, struct drm_minor *minor) > > filp->private_data = priv; > priv->filp = filp; > - priv->uid = current_euid(); > priv->pid = get_pid(task_pid(current)); > priv->minor = minor; > > diff --git a/drivers/gpu/drm/drm_info.c b/drivers/gpu/drm/drm_info.c > index 9ae353f..247ba2b 100644 > --- a/drivers/gpu/drm/drm_info.c > +++ b/drivers/gpu/drm/drm_info.c > @@ -98,13 +99,14 @@ int drm_clients_info(struct seq_file *m, void *data) > > rcu_read_lock(); /* locks pid_task()->comm */ > task = pid_task(priv->pid, PIDTYPE_PID); > + uid = priv->filp ? priv->filp->f_cred->euid : GLOBAL_ROOT_UID; uid = task_euid(task); > seq_printf(m, "%20s %5d %3d %c %c %5d %10u\n", > task ? task->comm : "<unknown>", > pid_vnr(priv->pid), > priv->minor->index, > drm_is_current_master(priv) ? 'y' : 'n', > priv->authenticated ? 'y' : 'n', > - from_kuid_munged(seq_user_ns(m), priv->uid), > + from_kuid_munged(seq_user_ns(m), uid), from_kuid_munged(seq_user_ns(m), task_euid(task)), Just fits. -Chris
On Wed, Aug 03, 2016 at 08:04:26PM +0200, David Herrmann wrote: > @@ -98,13 +99,14 @@ int drm_clients_info(struct seq_file *m, void *data) > > rcu_read_lock(); /* locks pid_task()->comm */ > task = pid_task(priv->pid, PIDTYPE_PID); > + uid = priv->filp ? priv->filp->f_cred->euid : GLOBAL_ROOT_UID; > seq_printf(m, "%20s %5d %3d %c %c %5d %10u\n", > task ? task->comm : "<unknown>", > pid_vnr(priv->pid), > priv->minor->index, > drm_is_current_master(priv) ? 'y' : 'n', > priv->authenticated ? 'y' : 'n', > - from_kuid_munged(seq_user_ns(m), priv->uid), > + from_kuid_munged(seq_user_ns(m), uid), > priv->magic); > rcu_read_unlock(); > } > diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c > index 33af4a5..49cd835 100644 > --- a/drivers/gpu/drm/drm_ioctl.c > +++ b/drivers/gpu/drm/drm_ioctl.c > @@ -191,7 +191,9 @@ static int drm_getclient(struct drm_device *dev, void *data, > client->auth = file_priv->authenticated; > client->pid = pid_vnr(file_priv->pid); > client->uid = from_kuid_munged(current_user_ns(), > - file_priv->uid); > + file_priv->filp ? > + file_priv->filp->f_cred->euid : > + GLOBAL_ROOT_UID); Why can't we use task_euid(pid_task(file_priv->pid)) here as well? -Chris
Hey On Wed, Aug 3, 2016 at 9:01 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote: > On Wed, Aug 03, 2016 at 08:04:26PM +0200, David Herrmann wrote: >> @@ -98,13 +99,14 @@ int drm_clients_info(struct seq_file *m, void *data) >> >> rcu_read_lock(); /* locks pid_task()->comm */ >> task = pid_task(priv->pid, PIDTYPE_PID); >> + uid = priv->filp ? priv->filp->f_cred->euid : GLOBAL_ROOT_UID; >> seq_printf(m, "%20s %5d %3d %c %c %5d %10u\n", >> task ? task->comm : "<unknown>", >> pid_vnr(priv->pid), >> priv->minor->index, >> drm_is_current_master(priv) ? 'y' : 'n', >> priv->authenticated ? 'y' : 'n', >> - from_kuid_munged(seq_user_ns(m), priv->uid), >> + from_kuid_munged(seq_user_ns(m), uid), >> priv->magic); >> rcu_read_unlock(); >> } >> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c >> index 33af4a5..49cd835 100644 >> --- a/drivers/gpu/drm/drm_ioctl.c >> +++ b/drivers/gpu/drm/drm_ioctl.c >> @@ -191,7 +191,9 @@ static int drm_getclient(struct drm_device *dev, void *data, >> client->auth = file_priv->authenticated; >> client->pid = pid_vnr(file_priv->pid); >> client->uid = from_kuid_munged(current_user_ns(), >> - file_priv->uid); >> + file_priv->filp ? >> + file_priv->filp->f_cred->euid : >> + GLOBAL_ROOT_UID); > > Why can't we use task_euid(pid_task(file_priv->pid)) here as well? task_euid() changes semantics. With this patch I just tried to get rid of "filp" usage, but keep semantics, so the ABI does not suddently change. Note that both calls are actually changed in follow-ups. "uid" is just cleared to "overflowuid", since no-one ever looks at that field. And "pid" is set to "pid_vnr(current)". However, I explicitly split those patches to make sure it is easier to bisect. Thanks David
diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c index 323c238..e9d66f5 100644 --- a/drivers/gpu/drm/drm_fops.c +++ b/drivers/gpu/drm/drm_fops.c @@ -199,7 +199,6 @@ static int drm_open_helper(struct file *filp, struct drm_minor *minor) filp->private_data = priv; priv->filp = filp; - priv->uid = current_euid(); priv->pid = get_pid(task_pid(current)); priv->minor = minor; diff --git a/drivers/gpu/drm/drm_info.c b/drivers/gpu/drm/drm_info.c index 9ae353f..247ba2b 100644 --- a/drivers/gpu/drm/drm_info.c +++ b/drivers/gpu/drm/drm_info.c @@ -80,6 +80,7 @@ int drm_clients_info(struct seq_file *m, void *data) struct drm_info_node *node = (struct drm_info_node *) m->private; struct drm_device *dev = node->minor->dev; struct drm_file *priv; + kuid_t uid; seq_printf(m, "%20s %5s %3s master a %5s %10s\n", @@ -98,13 +99,14 @@ int drm_clients_info(struct seq_file *m, void *data) rcu_read_lock(); /* locks pid_task()->comm */ task = pid_task(priv->pid, PIDTYPE_PID); + uid = priv->filp ? priv->filp->f_cred->euid : GLOBAL_ROOT_UID; seq_printf(m, "%20s %5d %3d %c %c %5d %10u\n", task ? task->comm : "<unknown>", pid_vnr(priv->pid), priv->minor->index, drm_is_current_master(priv) ? 'y' : 'n', priv->authenticated ? 'y' : 'n', - from_kuid_munged(seq_user_ns(m), priv->uid), + from_kuid_munged(seq_user_ns(m), uid), priv->magic); rcu_read_unlock(); } diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c index 33af4a5..49cd835 100644 --- a/drivers/gpu/drm/drm_ioctl.c +++ b/drivers/gpu/drm/drm_ioctl.c @@ -191,7 +191,9 @@ static int drm_getclient(struct drm_device *dev, void *data, client->auth = file_priv->authenticated; client->pid = pid_vnr(file_priv->pid); client->uid = from_kuid_munged(current_user_ns(), - file_priv->uid); + file_priv->filp ? + file_priv->filp->f_cred->euid : + GLOBAL_ROOT_UID); client->magic = 0; client->iocs = 0; diff --git a/include/drm/drmP.h b/include/drm/drmP.h index d488a72..0f69f56 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -320,7 +320,6 @@ struct drm_file { unsigned is_master:1; struct pid *pid; - kuid_t uid; drm_magic_t magic; struct list_head lhead; struct drm_minor *minor;
Each DRM file-context caches the EUID of the process that opened the file. It is used exclusively for debugging purposes in /proc/dri/ and friends. Note, however, that "struct file" already caches the credentials of a process at open-time. So lets just use drm_file->filp->f_cred->euid if available. The pointer-chasing will not hurt us, since it is only about debugging, anyway. Check priv->filp for NULL before deref, to prepare for in-kernel contexts (if they ever appear). We should not rely on "struct file" contexts to be around at all times, anyway. Signed-off-by: David Herrmann <dh.herrmann@gmail.com> --- drivers/gpu/drm/drm_fops.c | 1 - drivers/gpu/drm/drm_info.c | 4 +++- drivers/gpu/drm/drm_ioctl.c | 4 +++- include/drm/drmP.h | 1 - 4 files changed, 6 insertions(+), 4 deletions(-)