From: Mark Yacoub <[email protected]>
[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 <[email protected]>
---
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 dfec479830e49..afedf7109d002 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 65bc9710a4702..700782f021b99 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 46becedf5b2fc..10403c9a73082 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -1168,6 +1168,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
*
@@ -1181,6 +1184,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.40.0.634.g4ca3ef3211-goog
On Fri, Apr 21, 2023 at 12:27:47PM -0400, Mark Yacoub wrote:
> From: Mark Yacoub <[email protected]>
>
> [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
This makes no sense at all, why would you want to disallow reading?
Userspace already knows the key, there's not much point in hiding it from
userspace?
Also for new uapi we need the igt patches and userspace, please link
those.
-Daniel
>
> Signed-off-by: Mark Yacoub <[email protected]>
> ---
> 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 dfec479830e49..afedf7109d002 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 65bc9710a4702..700782f021b99 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 46becedf5b2fc..10403c9a73082 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -1168,6 +1168,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
> *
> @@ -1181,6 +1184,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.40.0.634.g4ca3ef3211-goog
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
On Thu, Apr 27, 2023 at 5:59 AM Daniel Vetter <[email protected]> wrote:
>
> On Fri, Apr 21, 2023 at 12:27:47PM -0400, Mark Yacoub wrote:
> > From: Mark Yacoub <[email protected]>
> >
> > [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
>
> This makes no sense at all, why would you want to disallow reading?
> Userspace already knows the key, there's not much point in hiding it from
> userspace?
There are varying levels of trust amongst userspace applications. For
example, in CrOS we trust portions of Chrome to handle the key
securely, but would like to avoid access from other portions, or users
from exposing the key via modetest output. We could play whack-a-mole
and try to patch up all untrusted userspace, but that doesn't seem
like a scalable solution.
Sean
>
> Also for new uapi we need the igt patches and userspace, please link
> those.
> -Daniel
>
> >
> > Signed-off-by: Mark Yacoub <[email protected]>
> > ---
> > 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 dfec479830e49..afedf7109d002 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 65bc9710a4702..700782f021b99 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 46becedf5b2fc..10403c9a73082 100644
> > --- a/include/uapi/drm/drm_mode.h
> > +++ b/include/uapi/drm/drm_mode.h
> > @@ -1168,6 +1168,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
> > *
> > @@ -1181,6 +1184,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.40.0.634.g4ca3ef3211-goog
> >
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch