| Message ID | 20240903144955.144278-2-mary.guillemard@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/panthor: Restrict high priorities on group_create | expand |
On Tue, 3 Sep 2024 16:49:55 +0200 Mary Guillemard <mary.guillemard@collabora.com> wrote: > We were allowing any users to create a high priority group without any > permission checks. As a result, this was allowing possible denial of > service. > > We now only allow the DRM master or users with the CAP_SYS_NICE > capability to set higher priorities than PANTHOR_GROUP_PRIORITY_MEDIUM. > > As the sole user of that uAPI lives in Mesa and hardcode a value of > MEDIUM [1], this should be safe to do. > > Additionally, as those checks are performed at the ioctl level, > panthor_group_create now only check for priority level validity. > > [1]https://gitlab.freedesktop.org/mesa/mesa/-/blob/f390835074bdf162a63deb0311d1a6de527f9f89/src/gallium/drivers/panfrost/pan_csf.c#L1038 > > Signed-off-by: Mary Guillemard <mary.guillemard@collabora.com> > Fixes: de8548813824 ("drm/panthor: Add the scheduler logical block") > Cc: stable@vger.kernel.org Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com> > --- > drivers/gpu/drm/panthor/panthor_drv.c | 23 +++++++++++++++++++++++ > drivers/gpu/drm/panthor/panthor_sched.c | 2 +- > include/uapi/drm/panthor_drm.h | 6 +++++- > 3 files changed, 29 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/panthor/panthor_drv.c b/drivers/gpu/drm/panthor/panthor_drv.c > index b5e7b919f241..34182f67136c 100644 > --- a/drivers/gpu/drm/panthor/panthor_drv.c > +++ b/drivers/gpu/drm/panthor/panthor_drv.c > @@ -10,6 +10,7 @@ > #include <linux/platform_device.h> > #include <linux/pm_runtime.h> > > +#include <drm/drm_auth.h> > #include <drm/drm_debugfs.h> > #include <drm/drm_drv.h> > #include <drm/drm_exec.h> > @@ -996,6 +997,24 @@ static int panthor_ioctl_group_destroy(struct drm_device *ddev, void *data, > return panthor_group_destroy(pfile, args->group_handle); > } > > +static int group_priority_permit(struct drm_file *file, > + u8 priority) > +{ > + /* Ensure that priority is valid */ > + if (priority > PANTHOR_GROUP_PRIORITY_HIGH) > + return -EINVAL; > + > + /* Medium priority and below are always allowed */ > + if (priority <= PANTHOR_GROUP_PRIORITY_MEDIUM) > + return 0; > + > + /* Higher priorities require CAP_SYS_NICE or DRM_MASTER */ > + if (capable(CAP_SYS_NICE) || drm_is_current_master(file)) > + return 0; > + > + return -EACCES; > +} > + > static int panthor_ioctl_group_create(struct drm_device *ddev, void *data, > struct drm_file *file) > { > @@ -1011,6 +1030,10 @@ static int panthor_ioctl_group_create(struct drm_device *ddev, void *data, > if (ret) > return ret; > > + ret = group_priority_permit(file, args->priority); > + if (ret) > + return ret; > + > ret = panthor_group_create(pfile, args, queue_args); > if (ret >= 0) { > args->group_handle = ret; > diff --git a/drivers/gpu/drm/panthor/panthor_sched.c b/drivers/gpu/drm/panthor/panthor_sched.c > index c426a392b081..91a31b70c037 100644 > --- a/drivers/gpu/drm/panthor/panthor_sched.c > +++ b/drivers/gpu/drm/panthor/panthor_sched.c > @@ -3092,7 +3092,7 @@ int panthor_group_create(struct panthor_file *pfile, > if (group_args->pad) > return -EINVAL; > > - if (group_args->priority > PANTHOR_CSG_PRIORITY_HIGH) > + if (group_args->priority >= PANTHOR_CSG_PRIORITY_COUNT) > return -EINVAL; > > if ((group_args->compute_core_mask & ~ptdev->gpu_info.shader_present) || > diff --git a/include/uapi/drm/panthor_drm.h b/include/uapi/drm/panthor_drm.h > index 926b1deb1116..e23a7f9b0eac 100644 > --- a/include/uapi/drm/panthor_drm.h > +++ b/include/uapi/drm/panthor_drm.h > @@ -692,7 +692,11 @@ enum drm_panthor_group_priority { > /** @PANTHOR_GROUP_PRIORITY_MEDIUM: Medium priority group. */ > PANTHOR_GROUP_PRIORITY_MEDIUM, > > - /** @PANTHOR_GROUP_PRIORITY_HIGH: High priority group. */ > + /** > + * @PANTHOR_GROUP_PRIORITY_HIGH: High priority group. > + * > + * Requires CAP_SYS_NICE or DRM_MASTER. > + */ > PANTHOR_GROUP_PRIORITY_HIGH, > }; > > > base-commit: a15710027afb40c7c1e352902fa5b8c949f021de
On Tue, 3 Sep 2024 16:49:55 +0200 Mary Guillemard <mary.guillemard@collabora.com> wrote: > We were allowing any users to create a high priority group without any > permission checks. As a result, this was allowing possible denial of > service. > > We now only allow the DRM master or users with the CAP_SYS_NICE > capability to set higher priorities than PANTHOR_GROUP_PRIORITY_MEDIUM. > > As the sole user of that uAPI lives in Mesa and hardcode a value of > MEDIUM [1], this should be safe to do. > > Additionally, as those checks are performed at the ioctl level, > panthor_group_create now only check for priority level validity. > > [1]https://gitlab.freedesktop.org/mesa/mesa/-/blob/f390835074bdf162a63deb0311d1a6de527f9f89/src/gallium/drivers/panfrost/pan_csf.c#L1038 > > Signed-off-by: Mary Guillemard <mary.guillemard@collabora.com> > Fixes: de8548813824 ("drm/panthor: Add the scheduler logical block") > Cc: stable@vger.kernel.org Queued to drm-misc-fixes. Thanks! > --- > drivers/gpu/drm/panthor/panthor_drv.c | 23 +++++++++++++++++++++++ > drivers/gpu/drm/panthor/panthor_sched.c | 2 +- > include/uapi/drm/panthor_drm.h | 6 +++++- > 3 files changed, 29 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/panthor/panthor_drv.c b/drivers/gpu/drm/panthor/panthor_drv.c > index b5e7b919f241..34182f67136c 100644 > --- a/drivers/gpu/drm/panthor/panthor_drv.c > +++ b/drivers/gpu/drm/panthor/panthor_drv.c > @@ -10,6 +10,7 @@ > #include <linux/platform_device.h> > #include <linux/pm_runtime.h> > > +#include <drm/drm_auth.h> > #include <drm/drm_debugfs.h> > #include <drm/drm_drv.h> > #include <drm/drm_exec.h> > @@ -996,6 +997,24 @@ static int panthor_ioctl_group_destroy(struct drm_device *ddev, void *data, > return panthor_group_destroy(pfile, args->group_handle); > } > > +static int group_priority_permit(struct drm_file *file, > + u8 priority) > +{ > + /* Ensure that priority is valid */ > + if (priority > PANTHOR_GROUP_PRIORITY_HIGH) > + return -EINVAL; > + > + /* Medium priority and below are always allowed */ > + if (priority <= PANTHOR_GROUP_PRIORITY_MEDIUM) > + return 0; > + > + /* Higher priorities require CAP_SYS_NICE or DRM_MASTER */ > + if (capable(CAP_SYS_NICE) || drm_is_current_master(file)) > + return 0; > + > + return -EACCES; > +} > + > static int panthor_ioctl_group_create(struct drm_device *ddev, void *data, > struct drm_file *file) > { > @@ -1011,6 +1030,10 @@ static int panthor_ioctl_group_create(struct drm_device *ddev, void *data, > if (ret) > return ret; > > + ret = group_priority_permit(file, args->priority); > + if (ret) > + return ret; > + > ret = panthor_group_create(pfile, args, queue_args); > if (ret >= 0) { > args->group_handle = ret; > diff --git a/drivers/gpu/drm/panthor/panthor_sched.c b/drivers/gpu/drm/panthor/panthor_sched.c > index c426a392b081..91a31b70c037 100644 > --- a/drivers/gpu/drm/panthor/panthor_sched.c > +++ b/drivers/gpu/drm/panthor/panthor_sched.c > @@ -3092,7 +3092,7 @@ int panthor_group_create(struct panthor_file *pfile, > if (group_args->pad) > return -EINVAL; > > - if (group_args->priority > PANTHOR_CSG_PRIORITY_HIGH) > + if (group_args->priority >= PANTHOR_CSG_PRIORITY_COUNT) > return -EINVAL; > > if ((group_args->compute_core_mask & ~ptdev->gpu_info.shader_present) || > diff --git a/include/uapi/drm/panthor_drm.h b/include/uapi/drm/panthor_drm.h > index 926b1deb1116..e23a7f9b0eac 100644 > --- a/include/uapi/drm/panthor_drm.h > +++ b/include/uapi/drm/panthor_drm.h > @@ -692,7 +692,11 @@ enum drm_panthor_group_priority { > /** @PANTHOR_GROUP_PRIORITY_MEDIUM: Medium priority group. */ > PANTHOR_GROUP_PRIORITY_MEDIUM, > > - /** @PANTHOR_GROUP_PRIORITY_HIGH: High priority group. */ > + /** > + * @PANTHOR_GROUP_PRIORITY_HIGH: High priority group. > + * > + * Requires CAP_SYS_NICE or DRM_MASTER. > + */ > PANTHOR_GROUP_PRIORITY_HIGH, > }; > > > base-commit: a15710027afb40c7c1e352902fa5b8c949f021de
diff --git a/drivers/gpu/drm/panthor/panthor_drv.c b/drivers/gpu/drm/panthor/panthor_drv.c index b5e7b919f241..34182f67136c 100644 --- a/drivers/gpu/drm/panthor/panthor_drv.c +++ b/drivers/gpu/drm/panthor/panthor_drv.c @@ -10,6 +10,7 @@ #include <linux/platform_device.h> #include <linux/pm_runtime.h> +#include <drm/drm_auth.h> #include <drm/drm_debugfs.h> #include <drm/drm_drv.h> #include <drm/drm_exec.h> @@ -996,6 +997,24 @@ static int panthor_ioctl_group_destroy(struct drm_device *ddev, void *data, return panthor_group_destroy(pfile, args->group_handle); } +static int group_priority_permit(struct drm_file *file, + u8 priority) +{ + /* Ensure that priority is valid */ + if (priority > PANTHOR_GROUP_PRIORITY_HIGH) + return -EINVAL; + + /* Medium priority and below are always allowed */ + if (priority <= PANTHOR_GROUP_PRIORITY_MEDIUM) + return 0; + + /* Higher priorities require CAP_SYS_NICE or DRM_MASTER */ + if (capable(CAP_SYS_NICE) || drm_is_current_master(file)) + return 0; + + return -EACCES; +} + static int panthor_ioctl_group_create(struct drm_device *ddev, void *data, struct drm_file *file) { @@ -1011,6 +1030,10 @@ static int panthor_ioctl_group_create(struct drm_device *ddev, void *data, if (ret) return ret; + ret = group_priority_permit(file, args->priority); + if (ret) + return ret; + ret = panthor_group_create(pfile, args, queue_args); if (ret >= 0) { args->group_handle = ret; diff --git a/drivers/gpu/drm/panthor/panthor_sched.c b/drivers/gpu/drm/panthor/panthor_sched.c index c426a392b081..91a31b70c037 100644 --- a/drivers/gpu/drm/panthor/panthor_sched.c +++ b/drivers/gpu/drm/panthor/panthor_sched.c @@ -3092,7 +3092,7 @@ int panthor_group_create(struct panthor_file *pfile, if (group_args->pad) return -EINVAL; - if (group_args->priority > PANTHOR_CSG_PRIORITY_HIGH) + if (group_args->priority >= PANTHOR_CSG_PRIORITY_COUNT) return -EINVAL; if ((group_args->compute_core_mask & ~ptdev->gpu_info.shader_present) || diff --git a/include/uapi/drm/panthor_drm.h b/include/uapi/drm/panthor_drm.h index 926b1deb1116..e23a7f9b0eac 100644 --- a/include/uapi/drm/panthor_drm.h +++ b/include/uapi/drm/panthor_drm.h @@ -692,7 +692,11 @@ enum drm_panthor_group_priority { /** @PANTHOR_GROUP_PRIORITY_MEDIUM: Medium priority group. */ PANTHOR_GROUP_PRIORITY_MEDIUM, - /** @PANTHOR_GROUP_PRIORITY_HIGH: High priority group. */ + /** + * @PANTHOR_GROUP_PRIORITY_HIGH: High priority group. + * + * Requires CAP_SYS_NICE or DRM_MASTER. + */ PANTHOR_GROUP_PRIORITY_HIGH, };
We were allowing any users to create a high priority group without any permission checks. As a result, this was allowing possible denial of service. We now only allow the DRM master or users with the CAP_SYS_NICE capability to set higher priorities than PANTHOR_GROUP_PRIORITY_MEDIUM. As the sole user of that uAPI lives in Mesa and hardcode a value of MEDIUM [1], this should be safe to do. Additionally, as those checks are performed at the ioctl level, panthor_group_create now only check for priority level validity. [1]https://gitlab.freedesktop.org/mesa/mesa/-/blob/f390835074bdf162a63deb0311d1a6de527f9f89/src/gallium/drivers/panfrost/pan_csf.c#L1038 Signed-off-by: Mary Guillemard <mary.guillemard@collabora.com> Fixes: de8548813824 ("drm/panthor: Add the scheduler logical block") Cc: stable@vger.kernel.org --- drivers/gpu/drm/panthor/panthor_drv.c | 23 +++++++++++++++++++++++ drivers/gpu/drm/panthor/panthor_sched.c | 2 +- include/uapi/drm/panthor_drm.h | 6 +++++- 3 files changed, 29 insertions(+), 2 deletions(-) base-commit: a15710027afb40c7c1e352902fa5b8c949f021de