* Re: [PATCH] drm: Create support for Write-Only property blob
2022-05-10 19:08 [PATCH] drm: Create support for Write-Only property blob Mark Yacoub
@ 2022-05-12 14:22 ` Mark Yacoub
2022-05-16 15:10 ` Mark Yacoub
2022-05-17 7:53 ` Jani Nikula
2 siblings, 0 replies; 8+ messages in thread
From: Mark Yacoub @ 2022-05-12 14:22 UTC (permalink / raw)
Cc: seanpaul, markyacoub, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Daniel Vetter, dri-devel,
linux-kernel
friendly ping :)
On Tue, May 10, 2022 at 3:08 PM Mark Yacoub <markyacoub@chromium.org> wrote:
>
> [Why]
> User space might need to inject data into the kernel without allowing it
> to be read again by any user space.
> An example of where this is particularly useful is secret keys fetched
> by user space and injected into the kernel to enable content protection.
>
> [How]
> Create a DRM_MODE_CREATE_BLOB_WRITE_ONLY flag used by user space to
> create a blob and mark the blob as write only.
> On reading back the blob, data will be not be copied if it's a write
> only blob
>
> Signed-off-by: Mark Yacoub <markyacoub@chromium.org>
>
> ---
> drivers/gpu/drm/drm_property.c | 3 ++-
> include/drm/drm_property.h | 2 ++
> include/uapi/drm/drm_mode.h | 6 ++++++
> 3 files changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_property.c b/drivers/gpu/drm/drm_property.c
> index dfec479830e4..afedf7109d00 100644
> --- a/drivers/gpu/drm/drm_property.c
> +++ b/drivers/gpu/drm/drm_property.c
> @@ -765,7 +765,7 @@ int drm_mode_getblob_ioctl(struct drm_device *dev,
> if (!blob)
> return -ENOENT;
>
> - if (out_resp->length == blob->length) {
> + if (out_resp->length == blob->length && !blob->is_write_only) {
> if (copy_to_user(u64_to_user_ptr(out_resp->data),
> blob->data,
> blob->length)) {
> @@ -800,6 +800,7 @@ int drm_mode_createblob_ioctl(struct drm_device *dev,
> ret = -EFAULT;
> goto out_blob;
> }
> + blob->is_write_only = out_resp->flags & DRM_MODE_CREATE_BLOB_WRITE_ONLY;
>
> /* Dropping the lock between create_blob and our access here is safe
> * as only the same file_priv can remove the blob; at this point, it is
> diff --git a/include/drm/drm_property.h b/include/drm/drm_property.h
> index 65bc9710a470..700782f021b9 100644
> --- a/include/drm/drm_property.h
> +++ b/include/drm/drm_property.h
> @@ -205,6 +205,7 @@ struct drm_property {
> * &drm_mode_config.property_blob_list.
> * @head_file: entry on the per-file blob list in &drm_file.blobs list.
> * @length: size of the blob in bytes, invariant over the lifetime of the object
> + * @is_write_only: user space can't read the blob data.
> * @data: actual data, embedded at the end of this structure
> *
> * Blobs are used to store bigger values than what fits directly into the 64
> @@ -219,6 +220,7 @@ struct drm_property_blob {
> struct list_head head_global;
> struct list_head head_file;
> size_t length;
> + bool is_write_only;
> void *data;
> };
>
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index 0a0d56a6158e..de192d3813e9 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -1107,6 +1107,9 @@ struct drm_format_modifier {
> __u64 modifier;
> };
>
> +#define DRM_MODE_CREATE_BLOB_WRITE_ONLY \
> + (1 << 0) /* data of the blob can't be read by user space */
> +
> /**
> * struct drm_mode_create_blob - Create New blob property
> *
> @@ -1120,6 +1123,9 @@ struct drm_mode_create_blob {
> __u32 length;
> /** @blob_id: Return: new property ID. */
> __u32 blob_id;
> + /** Flags for special handling. */
> + __u32 flags;
> + __u32 pad;
> };
>
> /**
> --
> 2.36.0.512.ge40c2bad7a-goog
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] drm: Create support for Write-Only property blob
2022-05-10 19:08 [PATCH] drm: Create support for Write-Only property blob Mark Yacoub
2022-05-12 14:22 ` Mark Yacoub
@ 2022-05-16 15:10 ` Mark Yacoub
2022-05-17 7:53 ` Jani Nikula
2 siblings, 0 replies; 8+ messages in thread
From: Mark Yacoub @ 2022-05-16 15:10 UTC (permalink / raw)
Cc: seanpaul, markyacoub, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Daniel Vetter, dri-devel,
linux-kernel
friendly ping :))
On Tue, May 10, 2022 at 3:08 PM Mark Yacoub <markyacoub@chromium.org> wrote:
>
> [Why]
> User space might need to inject data into the kernel without allowing it
> to be read again by any user space.
> An example of where this is particularly useful is secret keys fetched
> by user space and injected into the kernel to enable content protection.
>
> [How]
> Create a DRM_MODE_CREATE_BLOB_WRITE_ONLY flag used by user space to
> create a blob and mark the blob as write only.
> On reading back the blob, data will be not be copied if it's a write
> only blob
>
> Signed-off-by: Mark Yacoub <markyacoub@chromium.org>
>
> ---
> drivers/gpu/drm/drm_property.c | 3 ++-
> include/drm/drm_property.h | 2 ++
> include/uapi/drm/drm_mode.h | 6 ++++++
> 3 files changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_property.c b/drivers/gpu/drm/drm_property.c
> index dfec479830e4..afedf7109d00 100644
> --- a/drivers/gpu/drm/drm_property.c
> +++ b/drivers/gpu/drm/drm_property.c
> @@ -765,7 +765,7 @@ int drm_mode_getblob_ioctl(struct drm_device *dev,
> if (!blob)
> return -ENOENT;
>
> - if (out_resp->length == blob->length) {
> + if (out_resp->length == blob->length && !blob->is_write_only) {
> if (copy_to_user(u64_to_user_ptr(out_resp->data),
> blob->data,
> blob->length)) {
> @@ -800,6 +800,7 @@ int drm_mode_createblob_ioctl(struct drm_device *dev,
> ret = -EFAULT;
> goto out_blob;
> }
> + blob->is_write_only = out_resp->flags & DRM_MODE_CREATE_BLOB_WRITE_ONLY;
>
> /* Dropping the lock between create_blob and our access here is safe
> * as only the same file_priv can remove the blob; at this point, it is
> diff --git a/include/drm/drm_property.h b/include/drm/drm_property.h
> index 65bc9710a470..700782f021b9 100644
> --- a/include/drm/drm_property.h
> +++ b/include/drm/drm_property.h
> @@ -205,6 +205,7 @@ struct drm_property {
> * &drm_mode_config.property_blob_list.
> * @head_file: entry on the per-file blob list in &drm_file.blobs list.
> * @length: size of the blob in bytes, invariant over the lifetime of the object
> + * @is_write_only: user space can't read the blob data.
> * @data: actual data, embedded at the end of this structure
> *
> * Blobs are used to store bigger values than what fits directly into the 64
> @@ -219,6 +220,7 @@ struct drm_property_blob {
> struct list_head head_global;
> struct list_head head_file;
> size_t length;
> + bool is_write_only;
> void *data;
> };
>
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index 0a0d56a6158e..de192d3813e9 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -1107,6 +1107,9 @@ struct drm_format_modifier {
> __u64 modifier;
> };
>
> +#define DRM_MODE_CREATE_BLOB_WRITE_ONLY \
> + (1 << 0) /* data of the blob can't be read by user space */
> +
> /**
> * struct drm_mode_create_blob - Create New blob property
> *
> @@ -1120,6 +1123,9 @@ struct drm_mode_create_blob {
> __u32 length;
> /** @blob_id: Return: new property ID. */
> __u32 blob_id;
> + /** Flags for special handling. */
> + __u32 flags;
> + __u32 pad;
> };
>
> /**
> --
> 2.36.0.512.ge40c2bad7a-goog
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] drm: Create support for Write-Only property blob
2022-05-10 19:08 [PATCH] drm: Create support for Write-Only property blob Mark Yacoub
2022-05-12 14:22 ` Mark Yacoub
2022-05-16 15:10 ` Mark Yacoub
@ 2022-05-17 7:53 ` Jani Nikula
2022-05-25 19:31 ` Mark Yacoub
2 siblings, 1 reply; 8+ messages in thread
From: Jani Nikula @ 2022-05-17 7:53 UTC (permalink / raw)
To: Mark Yacoub
Cc: David Airlie, markyacoub, linux-kernel, seanpaul, dri-devel,
Thomas Zimmermann, markyacoub
On Tue, 10 May 2022, Mark Yacoub <markyacoub@chromium.org> wrote:
> [Why]
> User space might need to inject data into the kernel without allowing it
> to be read again by any user space.
> An example of where this is particularly useful is secret keys fetched
> by user space and injected into the kernel to enable content protection.
I think we're going to need more than an example in the commit
message. See Documentation/gpu/drm-uapi.rst.
BR,
Jani.
>
> [How]
> Create a DRM_MODE_CREATE_BLOB_WRITE_ONLY flag used by user space to
> create a blob and mark the blob as write only.
> On reading back the blob, data will be not be copied if it's a write
> only blob
>
> Signed-off-by: Mark Yacoub <markyacoub@chromium.org>
>
> ---
> drivers/gpu/drm/drm_property.c | 3 ++-
> include/drm/drm_property.h | 2 ++
> include/uapi/drm/drm_mode.h | 6 ++++++
> 3 files changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_property.c b/drivers/gpu/drm/drm_property.c
> index dfec479830e4..afedf7109d00 100644
> --- a/drivers/gpu/drm/drm_property.c
> +++ b/drivers/gpu/drm/drm_property.c
> @@ -765,7 +765,7 @@ int drm_mode_getblob_ioctl(struct drm_device *dev,
> if (!blob)
> return -ENOENT;
>
> - if (out_resp->length == blob->length) {
> + if (out_resp->length == blob->length && !blob->is_write_only) {
> if (copy_to_user(u64_to_user_ptr(out_resp->data),
> blob->data,
> blob->length)) {
> @@ -800,6 +800,7 @@ int drm_mode_createblob_ioctl(struct drm_device *dev,
> ret = -EFAULT;
> goto out_blob;
> }
> + blob->is_write_only = out_resp->flags & DRM_MODE_CREATE_BLOB_WRITE_ONLY;
>
> /* Dropping the lock between create_blob and our access here is safe
> * as only the same file_priv can remove the blob; at this point, it is
> diff --git a/include/drm/drm_property.h b/include/drm/drm_property.h
> index 65bc9710a470..700782f021b9 100644
> --- a/include/drm/drm_property.h
> +++ b/include/drm/drm_property.h
> @@ -205,6 +205,7 @@ struct drm_property {
> * &drm_mode_config.property_blob_list.
> * @head_file: entry on the per-file blob list in &drm_file.blobs list.
> * @length: size of the blob in bytes, invariant over the lifetime of the object
> + * @is_write_only: user space can't read the blob data.
> * @data: actual data, embedded at the end of this structure
> *
> * Blobs are used to store bigger values than what fits directly into the 64
> @@ -219,6 +220,7 @@ struct drm_property_blob {
> struct list_head head_global;
> struct list_head head_file;
> size_t length;
> + bool is_write_only;
> void *data;
> };
>
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index 0a0d56a6158e..de192d3813e9 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -1107,6 +1107,9 @@ struct drm_format_modifier {
> __u64 modifier;
> };
>
> +#define DRM_MODE_CREATE_BLOB_WRITE_ONLY \
> + (1 << 0) /* data of the blob can't be read by user space */
> +
> /**
> * struct drm_mode_create_blob - Create New blob property
> *
> @@ -1120,6 +1123,9 @@ struct drm_mode_create_blob {
> __u32 length;
> /** @blob_id: Return: new property ID. */
> __u32 blob_id;
> + /** Flags for special handling. */
> + __u32 flags;
> + __u32 pad;
> };
>
> /**
--
Jani Nikula, Intel Open Source Graphics Center
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] drm: Create support for Write-Only property blob
2022-05-17 7:53 ` Jani Nikula
@ 2022-05-25 19:31 ` Mark Yacoub
2022-06-29 16:09 ` Mark Yacoub
0 siblings, 1 reply; 8+ messages in thread
From: Mark Yacoub @ 2022-05-25 19:31 UTC (permalink / raw)
To: Jani Nikula
Cc: David Airlie, linux-kernel, seanpaul, dri-devel,
Thomas Zimmermann, markyacoub
Hi Jani, thanks for your review. I got all the user space
implementation ready to see it in context.
libdrm patch to wrap this functionality:
https://www.spinics.net/lists/dri-devel/msg347318.html
Chromium user space implementation making direct use of the new prop flag:
crrev.com/c/3655850
The first call made to such functionality is in
https://chromium-review.googlesource.com/c/chromium/src/+/3655850/2/ui/display/manager/content_protection_key_manager.cc#137
where the call stack flows to the libdrm wrapper call at
https://chromium-review.googlesource.com/c/chromium/src/+/3655850/2/ui/ozone/platform/drm/gpu/drm_display.cc#203
I also wrote an IGT test to verify the intended behavior:
https://patchwork.freedesktop.org/patch/487331/?series=104373&rev=1
Let me know if I would need to update the commit message with any of
the aforementioned context.
Thanks!
-Mark Yacoub
On Tue, May 17, 2022 at 3:53 AM Jani Nikula <jani.nikula@linux.intel.com> wrote:
>
> On Tue, 10 May 2022, Mark Yacoub <markyacoub@chromium.org> wrote:
> > [Why]
> > User space might need to inject data into the kernel without allowing it
> > to be read again by any user space.
> > An example of where this is particularly useful is secret keys fetched
> > by user space and injected into the kernel to enable content protection.
>
> I think we're going to need more than an example in the commit
> message. See Documentation/gpu/drm-uapi.rst.
>
> BR,
> Jani.
>
>
> >
> > [How]
> > Create a DRM_MODE_CREATE_BLOB_WRITE_ONLY flag used by user space to
> > create a blob and mark the blob as write only.
> > On reading back the blob, data will be not be copied if it's a write
> > only blob
> >
> > Signed-off-by: Mark Yacoub <markyacoub@chromium.org>
> >
> > ---
> > drivers/gpu/drm/drm_property.c | 3 ++-
> > include/drm/drm_property.h | 2 ++
> > include/uapi/drm/drm_mode.h | 6 ++++++
> > 3 files changed, 10 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/drm_property.c b/drivers/gpu/drm/drm_property.c
> > index dfec479830e4..afedf7109d00 100644
> > --- a/drivers/gpu/drm/drm_property.c
> > +++ b/drivers/gpu/drm/drm_property.c
> > @@ -765,7 +765,7 @@ int drm_mode_getblob_ioctl(struct drm_device *dev,
> > if (!blob)
> > return -ENOENT;
> >
> > - if (out_resp->length == blob->length) {
> > + if (out_resp->length == blob->length && !blob->is_write_only) {
> > if (copy_to_user(u64_to_user_ptr(out_resp->data),
> > blob->data,
> > blob->length)) {
> > @@ -800,6 +800,7 @@ int drm_mode_createblob_ioctl(struct drm_device *dev,
> > ret = -EFAULT;
> > goto out_blob;
> > }
> > + blob->is_write_only = out_resp->flags & DRM_MODE_CREATE_BLOB_WRITE_ONLY;
> >
> > /* Dropping the lock between create_blob and our access here is safe
> > * as only the same file_priv can remove the blob; at this point, it is
> > diff --git a/include/drm/drm_property.h b/include/drm/drm_property.h
> > index 65bc9710a470..700782f021b9 100644
> > --- a/include/drm/drm_property.h
> > +++ b/include/drm/drm_property.h
> > @@ -205,6 +205,7 @@ struct drm_property {
> > * &drm_mode_config.property_blob_list.
> > * @head_file: entry on the per-file blob list in &drm_file.blobs list.
> > * @length: size of the blob in bytes, invariant over the lifetime of the object
> > + * @is_write_only: user space can't read the blob data.
> > * @data: actual data, embedded at the end of this structure
> > *
> > * Blobs are used to store bigger values than what fits directly into the 64
> > @@ -219,6 +220,7 @@ struct drm_property_blob {
> > struct list_head head_global;
> > struct list_head head_file;
> > size_t length;
> > + bool is_write_only;
> > void *data;
> > };
> >
> > diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> > index 0a0d56a6158e..de192d3813e9 100644
> > --- a/include/uapi/drm/drm_mode.h
> > +++ b/include/uapi/drm/drm_mode.h
> > @@ -1107,6 +1107,9 @@ struct drm_format_modifier {
> > __u64 modifier;
> > };
> >
> > +#define DRM_MODE_CREATE_BLOB_WRITE_ONLY \
> > + (1 << 0) /* data of the blob can't be read by user space */
> > +
> > /**
> > * struct drm_mode_create_blob - Create New blob property
> > *
> > @@ -1120,6 +1123,9 @@ struct drm_mode_create_blob {
> > __u32 length;
> > /** @blob_id: Return: new property ID. */
> > __u32 blob_id;
> > + /** Flags for special handling. */
> > + __u32 flags;
> > + __u32 pad;
> > };
> >
> > /**
>
> --
> Jani Nikula, Intel Open Source Graphics Center
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] drm: Create support for Write-Only property blob
2022-05-25 19:31 ` Mark Yacoub
@ 2022-06-29 16:09 ` Mark Yacoub
2022-06-30 8:58 ` Jani Nikula
0 siblings, 1 reply; 8+ messages in thread
From: Mark Yacoub @ 2022-06-29 16:09 UTC (permalink / raw)
To: Jani Nikula
Cc: David Airlie, linux-kernel, seanpaul, dri-devel,
Thomas Zimmermann, markyacoub
Hi Jani, let me know if you need more info or more changes are needed. Thanks!
On Wed, May 25, 2022 at 3:31 PM Mark Yacoub <markyacoub@chromium.org> wrote:
>
> Hi Jani, thanks for your review. I got all the user space
> implementation ready to see it in context.
>
> libdrm patch to wrap this functionality:
> https://www.spinics.net/lists/dri-devel/msg347318.html
>
> Chromium user space implementation making direct use of the new prop flag:
> crrev.com/c/3655850
> The first call made to such functionality is in
> https://chromium-review.googlesource.com/c/chromium/src/+/3655850/2/ui/display/manager/content_protection_key_manager.cc#137
> where the call stack flows to the libdrm wrapper call at
> https://chromium-review.googlesource.com/c/chromium/src/+/3655850/2/ui/ozone/platform/drm/gpu/drm_display.cc#203
>
> I also wrote an IGT test to verify the intended behavior:
> https://patchwork.freedesktop.org/patch/487331/?series=104373&rev=1
>
> Let me know if I would need to update the commit message with any of
> the aforementioned context.
>
> Thanks!
> -Mark Yacoub
>
> On Tue, May 17, 2022 at 3:53 AM Jani Nikula <jani.nikula@linux.intel.com> wrote:
> >
> > On Tue, 10 May 2022, Mark Yacoub <markyacoub@chromium.org> wrote:
> > > [Why]
> > > User space might need to inject data into the kernel without allowing it
> > > to be read again by any user space.
> > > An example of where this is particularly useful is secret keys fetched
> > > by user space and injected into the kernel to enable content protection.
> >
> > I think we're going to need more than an example in the commit
> > message. See Documentation/gpu/drm-uapi.rst.
> >
> > BR,
> > Jani.
> >
> >
> > >
> > > [How]
> > > Create a DRM_MODE_CREATE_BLOB_WRITE_ONLY flag used by user space to
> > > create a blob and mark the blob as write only.
> > > On reading back the blob, data will be not be copied if it's a write
> > > only blob
> > >
> > > Signed-off-by: Mark Yacoub <markyacoub@chromium.org>
> > >
> > > ---
> > > drivers/gpu/drm/drm_property.c | 3 ++-
> > > include/drm/drm_property.h | 2 ++
> > > include/uapi/drm/drm_mode.h | 6 ++++++
> > > 3 files changed, 10 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_property.c b/drivers/gpu/drm/drm_property.c
> > > index dfec479830e4..afedf7109d00 100644
> > > --- a/drivers/gpu/drm/drm_property.c
> > > +++ b/drivers/gpu/drm/drm_property.c
> > > @@ -765,7 +765,7 @@ int drm_mode_getblob_ioctl(struct drm_device *dev,
> > > if (!blob)
> > > return -ENOENT;
> > >
> > > - if (out_resp->length == blob->length) {
> > > + if (out_resp->length == blob->length && !blob->is_write_only) {
> > > if (copy_to_user(u64_to_user_ptr(out_resp->data),
> > > blob->data,
> > > blob->length)) {
> > > @@ -800,6 +800,7 @@ int drm_mode_createblob_ioctl(struct drm_device *dev,
> > > ret = -EFAULT;
> > > goto out_blob;
> > > }
> > > + blob->is_write_only = out_resp->flags & DRM_MODE_CREATE_BLOB_WRITE_ONLY;
> > >
> > > /* Dropping the lock between create_blob and our access here is safe
> > > * as only the same file_priv can remove the blob; at this point, it is
> > > diff --git a/include/drm/drm_property.h b/include/drm/drm_property.h
> > > index 65bc9710a470..700782f021b9 100644
> > > --- a/include/drm/drm_property.h
> > > +++ b/include/drm/drm_property.h
> > > @@ -205,6 +205,7 @@ struct drm_property {
> > > * &drm_mode_config.property_blob_list.
> > > * @head_file: entry on the per-file blob list in &drm_file.blobs list.
> > > * @length: size of the blob in bytes, invariant over the lifetime of the object
> > > + * @is_write_only: user space can't read the blob data.
> > > * @data: actual data, embedded at the end of this structure
> > > *
> > > * Blobs are used to store bigger values than what fits directly into the 64
> > > @@ -219,6 +220,7 @@ struct drm_property_blob {
> > > struct list_head head_global;
> > > struct list_head head_file;
> > > size_t length;
> > > + bool is_write_only;
> > > void *data;
> > > };
> > >
> > > diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> > > index 0a0d56a6158e..de192d3813e9 100644
> > > --- a/include/uapi/drm/drm_mode.h
> > > +++ b/include/uapi/drm/drm_mode.h
> > > @@ -1107,6 +1107,9 @@ struct drm_format_modifier {
> > > __u64 modifier;
> > > };
> > >
> > > +#define DRM_MODE_CREATE_BLOB_WRITE_ONLY \
> > > + (1 << 0) /* data of the blob can't be read by user space */
> > > +
> > > /**
> > > * struct drm_mode_create_blob - Create New blob property
> > > *
> > > @@ -1120,6 +1123,9 @@ struct drm_mode_create_blob {
> > > __u32 length;
> > > /** @blob_id: Return: new property ID. */
> > > __u32 blob_id;
> > > + /** Flags for special handling. */
> > > + __u32 flags;
> > > + __u32 pad;
> > > };
> > >
> > > /**
> >
> > --
> > Jani Nikula, Intel Open Source Graphics Center
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] drm: Create support for Write-Only property blob
2022-06-29 16:09 ` Mark Yacoub
@ 2022-06-30 8:58 ` Jani Nikula
2022-07-08 17:36 ` Mark Yacoub
0 siblings, 1 reply; 8+ messages in thread
From: Jani Nikula @ 2022-06-30 8:58 UTC (permalink / raw)
To: Mark Yacoub
Cc: David Airlie, linux-kernel, seanpaul, dri-devel,
Thomas Zimmermann, markyacoub
On Wed, 29 Jun 2022, Mark Yacoub <markyacoub@chromium.org> wrote:
> Hi Jani, let me know if you need more info or more changes are needed. Thanks!
Sorry, I really haven't had the time to think this through.
At a glance, I'm unable to find the hdcp key property that this is being
used with. Am I missing something? Where's that created and used in the
kernel?
BR,
Jani.
>
> On Wed, May 25, 2022 at 3:31 PM Mark Yacoub <markyacoub@chromium.org> wrote:
>>
>> Hi Jani, thanks for your review. I got all the user space
>> implementation ready to see it in context.
>>
>> libdrm patch to wrap this functionality:
>> https://www.spinics.net/lists/dri-devel/msg347318.html
>>
>> Chromium user space implementation making direct use of the new prop flag:
>> crrev.com/c/3655850
>> The first call made to such functionality is in
>> https://chromium-review.googlesource.com/c/chromium/src/+/3655850/2/ui/display/manager/content_protection_key_manager.cc#137
>> where the call stack flows to the libdrm wrapper call at
>> https://chromium-review.googlesource.com/c/chromium/src/+/3655850/2/ui/ozone/platform/drm/gpu/drm_display.cc#203
>>
>> I also wrote an IGT test to verify the intended behavior:
>> https://patchwork.freedesktop.org/patch/487331/?series=104373&rev=1
>>
>> Let me know if I would need to update the commit message with any of
>> the aforementioned context.
>>
>> Thanks!
>> -Mark Yacoub
>>
>> On Tue, May 17, 2022 at 3:53 AM Jani Nikula <jani.nikula@linux.intel.com> wrote:
>> >
>> > On Tue, 10 May 2022, Mark Yacoub <markyacoub@chromium.org> wrote:
>> > > [Why]
>> > > User space might need to inject data into the kernel without allowing it
>> > > to be read again by any user space.
>> > > An example of where this is particularly useful is secret keys fetched
>> > > by user space and injected into the kernel to enable content protection.
>> >
>> > I think we're going to need more than an example in the commit
>> > message. See Documentation/gpu/drm-uapi.rst.
>> >
>> > BR,
>> > Jani.
>> >
>> >
>> > >
>> > > [How]
>> > > Create a DRM_MODE_CREATE_BLOB_WRITE_ONLY flag used by user space to
>> > > create a blob and mark the blob as write only.
>> > > On reading back the blob, data will be not be copied if it's a write
>> > > only blob
>> > >
>> > > Signed-off-by: Mark Yacoub <markyacoub@chromium.org>
>> > >
>> > > ---
>> > > drivers/gpu/drm/drm_property.c | 3 ++-
>> > > include/drm/drm_property.h | 2 ++
>> > > include/uapi/drm/drm_mode.h | 6 ++++++
>> > > 3 files changed, 10 insertions(+), 1 deletion(-)
>> > >
>> > > diff --git a/drivers/gpu/drm/drm_property.c b/drivers/gpu/drm/drm_property.c
>> > > index dfec479830e4..afedf7109d00 100644
>> > > --- a/drivers/gpu/drm/drm_property.c
>> > > +++ b/drivers/gpu/drm/drm_property.c
>> > > @@ -765,7 +765,7 @@ int drm_mode_getblob_ioctl(struct drm_device *dev,
>> > > if (!blob)
>> > > return -ENOENT;
>> > >
>> > > - if (out_resp->length == blob->length) {
>> > > + if (out_resp->length == blob->length && !blob->is_write_only) {
>> > > if (copy_to_user(u64_to_user_ptr(out_resp->data),
>> > > blob->data,
>> > > blob->length)) {
>> > > @@ -800,6 +800,7 @@ int drm_mode_createblob_ioctl(struct drm_device *dev,
>> > > ret = -EFAULT;
>> > > goto out_blob;
>> > > }
>> > > + blob->is_write_only = out_resp->flags & DRM_MODE_CREATE_BLOB_WRITE_ONLY;
>> > >
>> > > /* Dropping the lock between create_blob and our access here is safe
>> > > * as only the same file_priv can remove the blob; at this point, it is
>> > > diff --git a/include/drm/drm_property.h b/include/drm/drm_property.h
>> > > index 65bc9710a470..700782f021b9 100644
>> > > --- a/include/drm/drm_property.h
>> > > +++ b/include/drm/drm_property.h
>> > > @@ -205,6 +205,7 @@ struct drm_property {
>> > > * &drm_mode_config.property_blob_list.
>> > > * @head_file: entry on the per-file blob list in &drm_file.blobs list.
>> > > * @length: size of the blob in bytes, invariant over the lifetime of the object
>> > > + * @is_write_only: user space can't read the blob data.
>> > > * @data: actual data, embedded at the end of this structure
>> > > *
>> > > * Blobs are used to store bigger values than what fits directly into the 64
>> > > @@ -219,6 +220,7 @@ struct drm_property_blob {
>> > > struct list_head head_global;
>> > > struct list_head head_file;
>> > > size_t length;
>> > > + bool is_write_only;
>> > > void *data;
>> > > };
>> > >
>> > > diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
>> > > index 0a0d56a6158e..de192d3813e9 100644
>> > > --- a/include/uapi/drm/drm_mode.h
>> > > +++ b/include/uapi/drm/drm_mode.h
>> > > @@ -1107,6 +1107,9 @@ struct drm_format_modifier {
>> > > __u64 modifier;
>> > > };
>> > >
>> > > +#define DRM_MODE_CREATE_BLOB_WRITE_ONLY \
>> > > + (1 << 0) /* data of the blob can't be read by user space */
>> > > +
>> > > /**
>> > > * struct drm_mode_create_blob - Create New blob property
>> > > *
>> > > @@ -1120,6 +1123,9 @@ struct drm_mode_create_blob {
>> > > __u32 length;
>> > > /** @blob_id: Return: new property ID. */
>> > > __u32 blob_id;
>> > > + /** Flags for special handling. */
>> > > + __u32 flags;
>> > > + __u32 pad;
>> > > };
>> > >
>> > > /**
>> >
>> > --
>> > Jani Nikula, Intel Open Source Graphics Center
--
Jani Nikula, Intel Open Source Graphics Center
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] drm: Create support for Write-Only property blob
2022-06-30 8:58 ` Jani Nikula
@ 2022-07-08 17:36 ` Mark Yacoub
0 siblings, 0 replies; 8+ messages in thread
From: Mark Yacoub @ 2022-07-08 17:36 UTC (permalink / raw)
To: Jani Nikula
Cc: David Airlie, linux-kernel, seanpaul, dri-devel,
Thomas Zimmermann, markyacoub
Hi Jani,
There is still one piece missing in the puzzle to be done.
This is the patch for the HDCP implementation in the kernel needing a
secret key: https://patchwork.freedesktop.org/series/94623/
The implementation currently uses debugfs in testing. The next step is
to use the blob created instead to inject the key into the kernel,
So, so far what we have implemented in the user space creating the
blob and injecting it into the kernel, and the part where the key is
needed,
Thanks!
On Thu, Jun 30, 2022 at 4:58 AM Jani Nikula <jani.nikula@linux.intel.com> wrote:
>
> On Wed, 29 Jun 2022, Mark Yacoub <markyacoub@chromium.org> wrote:
> > Hi Jani, let me know if you need more info or more changes are needed. Thanks!
>
> Sorry, I really haven't had the time to think this through.
>
> At a glance, I'm unable to find the hdcp key property that this is being
> used with. Am I missing something? Where's that created and used in the
> kernel?
>
>
> BR,
> Jani.
>
>
>
> >
> > On Wed, May 25, 2022 at 3:31 PM Mark Yacoub <markyacoub@chromium.org> wrote:
> >>
> >> Hi Jani, thanks for your review. I got all the user space
> >> implementation ready to see it in context.
> >>
> >> libdrm patch to wrap this functionality:
> >> https://www.spinics.net/lists/dri-devel/msg347318.html
> >>
> >> Chromium user space implementation making direct use of the new prop flag:
> >> crrev.com/c/3655850
> >> The first call made to such functionality is in
> >> https://chromium-review.googlesource.com/c/chromium/src/+/3655850/2/ui/display/manager/content_protection_key_manager.cc#137
> >> where the call stack flows to the libdrm wrapper call at
> >> https://chromium-review.googlesource.com/c/chromium/src/+/3655850/2/ui/ozone/platform/drm/gpu/drm_display.cc#203
> >>
> >> I also wrote an IGT test to verify the intended behavior:
> >> https://patchwork.freedesktop.org/patch/487331/?series=104373&rev=1
> >>
> >> Let me know if I would need to update the commit message with any of
> >> the aforementioned context.
> >>
> >> Thanks!
> >> -Mark Yacoub
> >>
> >> On Tue, May 17, 2022 at 3:53 AM Jani Nikula <jani.nikula@linux.intel.com> wrote:
> >> >
> >> > On Tue, 10 May 2022, Mark Yacoub <markyacoub@chromium.org> wrote:
> >> > > [Why]
> >> > > User space might need to inject data into the kernel without allowing it
> >> > > to be read again by any user space.
> >> > > An example of where this is particularly useful is secret keys fetched
> >> > > by user space and injected into the kernel to enable content protection.
> >> >
> >> > I think we're going to need more than an example in the commit
> >> > message. See Documentation/gpu/drm-uapi.rst.
> >> >
> >> > BR,
> >> > Jani.
> >> >
> >> >
> >> > >
> >> > > [How]
> >> > > Create a DRM_MODE_CREATE_BLOB_WRITE_ONLY flag used by user space to
> >> > > create a blob and mark the blob as write only.
> >> > > On reading back the blob, data will be not be copied if it's a write
> >> > > only blob
> >> > >
> >> > > Signed-off-by: Mark Yacoub <markyacoub@chromium.org>
> >> > >
> >> > > ---
> >> > > drivers/gpu/drm/drm_property.c | 3 ++-
> >> > > include/drm/drm_property.h | 2 ++
> >> > > include/uapi/drm/drm_mode.h | 6 ++++++
> >> > > 3 files changed, 10 insertions(+), 1 deletion(-)
> >> > >
> >> > > diff --git a/drivers/gpu/drm/drm_property.c b/drivers/gpu/drm/drm_property.c
> >> > > index dfec479830e4..afedf7109d00 100644
> >> > > --- a/drivers/gpu/drm/drm_property.c
> >> > > +++ b/drivers/gpu/drm/drm_property.c
> >> > > @@ -765,7 +765,7 @@ int drm_mode_getblob_ioctl(struct drm_device *dev,
> >> > > if (!blob)
> >> > > return -ENOENT;
> >> > >
> >> > > - if (out_resp->length == blob->length) {
> >> > > + if (out_resp->length == blob->length && !blob->is_write_only) {
> >> > > if (copy_to_user(u64_to_user_ptr(out_resp->data),
> >> > > blob->data,
> >> > > blob->length)) {
> >> > > @@ -800,6 +800,7 @@ int drm_mode_createblob_ioctl(struct drm_device *dev,
> >> > > ret = -EFAULT;
> >> > > goto out_blob;
> >> > > }
> >> > > + blob->is_write_only = out_resp->flags & DRM_MODE_CREATE_BLOB_WRITE_ONLY;
> >> > >
> >> > > /* Dropping the lock between create_blob and our access here is safe
> >> > > * as only the same file_priv can remove the blob; at this point, it is
> >> > > diff --git a/include/drm/drm_property.h b/include/drm/drm_property.h
> >> > > index 65bc9710a470..700782f021b9 100644
> >> > > --- a/include/drm/drm_property.h
> >> > > +++ b/include/drm/drm_property.h
> >> > > @@ -205,6 +205,7 @@ struct drm_property {
> >> > > * &drm_mode_config.property_blob_list.
> >> > > * @head_file: entry on the per-file blob list in &drm_file.blobs list.
> >> > > * @length: size of the blob in bytes, invariant over the lifetime of the object
> >> > > + * @is_write_only: user space can't read the blob data.
> >> > > * @data: actual data, embedded at the end of this structure
> >> > > *
> >> > > * Blobs are used to store bigger values than what fits directly into the 64
> >> > > @@ -219,6 +220,7 @@ struct drm_property_blob {
> >> > > struct list_head head_global;
> >> > > struct list_head head_file;
> >> > > size_t length;
> >> > > + bool is_write_only;
> >> > > void *data;
> >> > > };
> >> > >
> >> > > diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> >> > > index 0a0d56a6158e..de192d3813e9 100644
> >> > > --- a/include/uapi/drm/drm_mode.h
> >> > > +++ b/include/uapi/drm/drm_mode.h
> >> > > @@ -1107,6 +1107,9 @@ struct drm_format_modifier {
> >> > > __u64 modifier;
> >> > > };
> >> > >
> >> > > +#define DRM_MODE_CREATE_BLOB_WRITE_ONLY \
> >> > > + (1 << 0) /* data of the blob can't be read by user space */
> >> > > +
> >> > > /**
> >> > > * struct drm_mode_create_blob - Create New blob property
> >> > > *
> >> > > @@ -1120,6 +1123,9 @@ struct drm_mode_create_blob {
> >> > > __u32 length;
> >> > > /** @blob_id: Return: new property ID. */
> >> > > __u32 blob_id;
> >> > > + /** Flags for special handling. */
> >> > > + __u32 flags;
> >> > > + __u32 pad;
> >> > > };
> >> > >
> >> > > /**
> >> >
> >> > --
> >> > Jani Nikula, Intel Open Source Graphics Center
>
> --
> Jani Nikula, Intel Open Source Graphics Center
^ permalink raw reply [flat|nested] 8+ messages in thread