| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <oc7nqx5gxrefaphpoyn7tsyhj2zcpbhwuxnhlgxtp6exet2ebz@wve2rz376pf4>
Date: Mon, 14 Apr 2025 21:41:13 +0100
From: Adrián Larumbe <adrian.larumbe@...labora.com>
To: Steven Price <steven.price@....com>
Cc: Boris Brezillon <boris.brezillon@...labora.com>,
Liviu Dudau <liviu.dudau@....com>, Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
Maxime Ripard <mripard@...nel.org>, Thomas Zimmermann <tzimmermann@...e.de>,
David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>,
Sumit Semwal <sumit.semwal@...aro.org>, Christian König <christian.koenig@....com>,
kernel@...labora.com, dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
linux-media@...r.kernel.org, linaro-mm-sig@...ts.linaro.org
Subject: Re: [PATCH v7 2/4] drm/panthor: Add driver IOCTL for setting BO
labels
On 14.04.2025 11:01, Steven Price wrote:
> On 11/04/2025 16:03, Adrián Larumbe wrote:
> > Allow UM to label a BO for which it possesses a DRM handle.
> >
> > Signed-off-by: Adrián Larumbe <adrian.larumbe@...labora.com>
> > Reviewed-by: Liviu Dudau <liviu.dudau@....com>
> > Reviewed-by: Boris Brezillon <boris.brezillon@...labora.com>
>
> Reviewed-by: Steven Price <steven.price@....com>
>
> Although very minor NITs below which you can consider.
>
> > ---
> > drivers/gpu/drm/panthor/panthor_drv.c | 42 ++++++++++++++++++++++++++-
> > drivers/gpu/drm/panthor/panthor_gem.h | 2 ++
> > include/uapi/drm/panthor_drm.h | 23 +++++++++++++++
> > 3 files changed, 66 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/panthor/panthor_drv.c b/drivers/gpu/drm/panthor/panthor_drv.c
> > index 06fe46e32073..983b24f1236c 100644
> > --- a/drivers/gpu/drm/panthor/panthor_drv.c
> > +++ b/drivers/gpu/drm/panthor/panthor_drv.c
> > @@ -1331,6 +1331,44 @@ static int panthor_ioctl_vm_get_state(struct drm_device *ddev, void *data,
> > return 0;
> > }
> >
> > +static int panthor_ioctl_bo_set_label(struct drm_device *ddev, void *data,
> > + struct drm_file *file)
> > +{
> > + struct drm_panthor_bo_set_label *args = data;
> > + struct drm_gem_object *obj;
> > + const char *label;
> > + int ret = 0;
> > +
> > + obj = drm_gem_object_lookup(file, args->handle);
> > + if (!obj)
> > + return -ENOENT;
> > +
> > + if (args->size && args->label) {
> > + if (args->size > PANTHOR_BO_LABEL_MAXLEN) {
> > + ret = -E2BIG;
> > + goto err_label;
> > + }
> > +
> > + label = strndup_user(u64_to_user_ptr(args->label), args->size);
> > + if (IS_ERR(label)) {
> > + ret = PTR_ERR(label);
> > + goto err_label;
> > + }
> > + } else if (args->size && !args->label) {
> > + ret = -EINVAL;
> > + goto err_label;
> > + } else {
> > + label = NULL;
> > + }
> > +
> > + panthor_gem_bo_set_label(obj, label);
> > +
> > +err_label:
> > + drm_gem_object_put(obj);
> > +
> > + return ret;
> > +}
> > +
> > static int
> > panthor_open(struct drm_device *ddev, struct drm_file *file)
> > {
> > @@ -1400,6 +1438,7 @@ static const struct drm_ioctl_desc panthor_drm_driver_ioctls[] = {
> > PANTHOR_IOCTL(TILER_HEAP_CREATE, tiler_heap_create, DRM_RENDER_ALLOW),
> > PANTHOR_IOCTL(TILER_HEAP_DESTROY, tiler_heap_destroy, DRM_RENDER_ALLOW),
> > PANTHOR_IOCTL(GROUP_SUBMIT, group_submit, DRM_RENDER_ALLOW),
> > + PANTHOR_IOCTL(BO_SET_LABEL, bo_set_label, DRM_RENDER_ALLOW),
> > };
> >
> > static int panthor_mmap(struct file *filp, struct vm_area_struct *vma)
> > @@ -1509,6 +1548,7 @@ static void panthor_debugfs_init(struct drm_minor *minor)
> > * - 1.2 - adds DEV_QUERY_GROUP_PRIORITIES_INFO query
> > * - adds PANTHOR_GROUP_PRIORITY_REALTIME priority
> > * - 1.3 - adds DRM_PANTHOR_GROUP_STATE_INNOCENT flag
> > + * - 1.4 - adds DRM_IOCTL_PANTHOR_BO_SET_LABEL ioctl
> > */
> > static const struct drm_driver panthor_drm_driver = {
> > .driver_features = DRIVER_RENDER | DRIVER_GEM | DRIVER_SYNCOBJ |
> > @@ -1522,7 +1562,7 @@ static const struct drm_driver panthor_drm_driver = {
> > .name = "panthor",
> > .desc = "Panthor DRM driver",
> > .major = 1,
> > - .minor = 3,
> > + .minor = 4,
> >
> > .gem_create_object = panthor_gem_create_object,
> > .gem_prime_import_sg_table = drm_gem_shmem_prime_import_sg_table,
> > diff --git a/drivers/gpu/drm/panthor/panthor_gem.h b/drivers/gpu/drm/panthor/panthor_gem.h
> > index af0d77338860..beba066b4974 100644
> > --- a/drivers/gpu/drm/panthor/panthor_gem.h
> > +++ b/drivers/gpu/drm/panthor/panthor_gem.h
> > @@ -13,6 +13,8 @@
> >
> > struct panthor_vm;
> >
> > +#define PANTHOR_BO_LABEL_MAXLEN PAGE_SIZE
> > +
> > /**
> > * struct panthor_gem_object - Driver specific GEM object.
> > */
> > diff --git a/include/uapi/drm/panthor_drm.h b/include/uapi/drm/panthor_drm.h
> > index 97e2c4510e69..12b1994499a9 100644
> > --- a/include/uapi/drm/panthor_drm.h
> > +++ b/include/uapi/drm/panthor_drm.h
> > @@ -127,6 +127,9 @@ enum drm_panthor_ioctl_id {
> >
> > /** @DRM_PANTHOR_TILER_HEAP_DESTROY: Destroy a tiler heap. */
> > DRM_PANTHOR_TILER_HEAP_DESTROY,
> > +
> > + /** @DRM_PANTHOR_BO_SET_LABEL: Label a BO. */
> > + DRM_PANTHOR_BO_SET_LABEL,
> > };
> >
> > /**
> > @@ -977,6 +980,24 @@ struct drm_panthor_tiler_heap_destroy {
> > __u32 pad;
> > };
> >
> > +/**
> > + * struct drm_panthor_bo_set_label - Arguments passed to DRM_IOCTL_PANTHOR_BO_SET_LABEL
> > + */
> > +struct drm_panthor_bo_set_label {
> > + /** @handle: Handle of the buffer object to label. */
> > + __u32 handle;
> > +
> > + /**
> > + * @size: Length of the label, including the NULL terminator.
> > + *
> > + * Cannot be greater than the OS page size.
> > + */
> > + __u32 size;
> > +
> > + /** @label: User pointer to a NULL-terminated string */
> > + __u64 label;
> > +};
>
> First very minor NIT:
> * NULL is a pointer, i.e. (void*)0
> * NUL is the ASCII code point '\0'.
> So it's a NUL-terminated string.
Fixed
> Second NIT: We don't actually need 'size' - since the string is
> NUL-terminated we can just strndup_user(__user_pointer__, PAGE_SIZE).
> As things stand we validate that strlen(label) < size <= PAGE_SIZE -
> which is a little odd (user space might as well just pass PAGE_SIZE
> rather than calculate the actual length).
The snag I see in this approach is that the only way to make sure
strlen(label) + 1 <= PAGE_SIZE would be doing something like
label = strndup_user(u64_to_user_ptr(args->label), args->size);
if (strlen(label) + 1 <= PAGE_SIZE) {
kfree(label)
return -E2BIG;
}
In the meantime, we've duplicated the string and traversed a whole page
of bytes, all to be discarded at once.
In this case, I think it's alright to expect some cooperation from UM
in supplying the actual size, although I'm really not an expert in
designing elegant uAPIs, so if you think this looks very odd I'd be
glad to replace it with.
Actually, as I was writing this, I realised that strndup_user() calls
strnlen_user(), which is publicly available for other drivers, so
I might check the length first, and if it falls within bounds, do
the actual user stringdup.
I shall also mention the size bound on the uAPI for the 'label' pointer.
> Thanks,
> Steve
>
> +
> /**
> * DRM_IOCTL_PANTHOR() - Build a Panthor IOCTL number
> * @__access: Access type. Must be R, W or RW.
> @@ -1019,6 +1040,8 @@ enum {
> DRM_IOCTL_PANTHOR(WR, TILER_HEAP_CREATE, tiler_heap_create),
> DRM_IOCTL_PANTHOR_TILER_HEAP_DESTROY =
> DRM_IOCTL_PANTHOR(WR, TILER_HEAP_DESTROY, tiler_heap_destroy),
> + DRM_IOCTL_PANTHOR_BO_SET_LABEL =
> + DRM_IOCTL_PANTHOR(WR, BO_SET_LABEL, bo_set_label),
> };
>
> #if defined(__cplusplus)
Adrian Larumbe
Powered by blists - more mailing lists