[PATCH v2 1/3] drm: Create support for Write-Only property blob

[PATCH v2 1/3] drm: Create support for Write-Only property blob

[Mark Yacoub]

From: Mark Yacoub <markyacoub@chromium.org>

[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 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

[PATCH v2 2/3] DRM: Create new Content Protection connector property

[Mark Yacoub]

From: Mark Yacoub <markyacoub@chromium.org>

[Why]
To enable Protected Content, some drivers require a key to be injected
from user space to enable HDCP on the connector.

[How]
Create new "Content Protection Property" of type "Blob"

Signed-off-by: Mark Yacoub <markyacoub@chromium.org>
---
 drivers/gpu/drm/drm_atomic_uapi.c | 9 +++++++++
 include/drm/drm_connector.h       | 6 ++++++
 include/drm/drm_mode_config.h     | 6 ++++++
 3 files changed, 21 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
index d867e7f9f2cd5..e20bc57cdb05c 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -749,6 +749,11 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector,
 		state->content_protection = val;
 	} else if (property == config->hdcp_content_type_property) {
 		state->hdcp_content_type = val;
+	} else if (property == config->content_protection_key_property) {
+		ret = drm_atomic_replace_property_blob_from_id(
+			dev, &state->content_protection_key, val, -1, -1,
+			&replaced);
+		return ret;
 	} else if (property == connector->colorspace_property) {
 		state->colorspace = val;
 	} else if (property == config->writeback_fb_id_property) {
@@ -843,6 +848,10 @@ drm_atomic_connector_get_property(struct drm_connector *connector,
 		*val = state->content_protection;
 	} else if (property == config->hdcp_content_type_property) {
 		*val = state->hdcp_content_type;
+	} else if (property == config->content_protection_key_property) {
+		*val = state->content_protection_key ?
+			       state->content_protection_key->base.id :
+			       0;
 	} else if (property == config->writeback_fb_id_property) {
 		/* Writeback framebuffer is one-shot, write and forget */
 		*val = 0;
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 7b5048516185c..2fbe51272bfeb 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -896,6 +896,12 @@ struct drm_connector_state {
 	 */
 	unsigned int content_protection;
 
+	/**
+	 * @content_protection_key: DRM blob property for holding the Content
+	 * Protection Key injected from user space.
+	 */
+	struct drm_property_blob *content_protection_key;
+
 	/**
 	 * @colorspace: State variable for Connector property to request
 	 * colorspace change on Sink. This is most commonly used to switch
diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
index e5b053001d22e..615d1e5f57562 100644
--- a/include/drm/drm_mode_config.h
+++ b/include/drm/drm_mode_config.h
@@ -887,6 +887,12 @@ struct drm_mode_config {
 	 */
 	struct drm_property *hdcp_content_type_property;
 
+	/**
+	 * @content_protection_key_property: DRM blob property that receives the 
+	 * content protection key from user space to be injected into the kernel.
+	 */
+	struct drm_property *content_protection_key_property;
+
 	/* dumb ioctl parameters */
 	uint32_t preferred_depth, prefer_shadow;
 
-- 
2.40.0.634.g4ca3ef3211-goog

[PATCH v2 3/3] dp_hdcp: Get the hdcp key from the connector prop

[Mark Yacoub]

From: Mark Yacoub <markyacoub@chromium.org>

[Why]
To support protected content, the driver requires a key.
Currently, it's being injected from debugfs, which is not super useful
to run a user space in the wild.

[How]
When the key is needed, fetch the "Content Protection Property" on the
connector and get the key blob. Verify that the size is valid and use
it.

Signed-off-by: Mark Yacoub <markyacoub@chromium.org>
---
 drivers/gpu/drm/msm/dp/dp_hdcp.c | 66 +++++++++++++++++++++++++++++---
 1 file changed, 61 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_hdcp.c b/drivers/gpu/drm/msm/dp/dp_hdcp.c
index 191340971f943..4321d245b36c9 100644
--- a/drivers/gpu/drm/msm/dp/dp_hdcp.c
+++ b/drivers/gpu/drm/msm/dp/dp_hdcp.c
@@ -117,19 +117,61 @@ static bool dp_hdcp_are_keys_valid(struct drm_connector *connector,
 	return FIELD_GET(DP_HDCP_KEY_STATUS, val) == DP_HDCP_KEY_STATUS_VALID;
 }
 
+static bool dp_hdcp_get_key_from_connector(struct drm_connector *connector,
+					   struct drm_bridge *bridge)
+{
+	struct drm_property_blob *key_blob;
+	u8 *raw_key;
+	int ret;
+	struct dp_hdcp *hdcp;
+	struct drm_device *dev = connector->dev;
+	struct drm_property *prop =
+		dev->mode_config.content_protection_key_property;
+
+	if (!prop)
+		return false;
+
+	key_blob = connector->state->content_protection_key;
+	if (!key_blob)
+		return false;
+
+	raw_key = key_blob->data;
+
+	if (key_blob->length !=
+	    DRM_HDCP_KSV_LEN + DP_HDCP_NUM_KEYS * DP_HDCP_KEY_LEN) {
+		drm_dbg_atomic(
+			dev,
+			"[CONNECTOR:%d:%s] Content Protection Key is a blob that we don't expect.\n",
+			connector->base.id, connector->name);
+		return false;
+	}
+
+	hdcp = dp_display_bridge_to_hdcp(bridge);
+	ret = dp_hdcp_ingest_key(hdcp, key_blob->data, key_blob->length);
+	if (ret)
+		return false;
+
+	return true;
+}
+
 static int dp_hdcp_load_keys(struct drm_connector *connector, void *driver_data)
 {
 	struct drm_bridge *bridge = (struct drm_bridge *)driver_data;
 	struct dp_hdcp *hdcp = dp_display_bridge_to_hdcp(bridge);
 	int i, ret = 0;
+	bool is_hdcp_key_valid;
 
 	mutex_lock(&hdcp->key_lock);
+	is_hdcp_key_valid = hdcp->key.valid;
+	mutex_unlock(&hdcp->key_lock);
 
-	if (!hdcp->key.valid) {
-		ret = -ENOENT;
-		goto out;
+	if (!is_hdcp_key_valid &&
+	    !dp_hdcp_get_key_from_connector(connector, bridge)) {
+		return -ENOENT;
 	}
 
+	mutex_lock(&hdcp->key_lock);
+
 	dp_catalog_hdcp_write_aksv(hdcp->catalog, hdcp->key.ksv.words);
 
 
@@ -139,7 +181,6 @@ static int dp_hdcp_load_keys(struct drm_connector *connector, void *driver_data)
 	}
 	dp_catalog_hdcp_post_write_key(hdcp->catalog);
 
-out:
 	mutex_unlock(&hdcp->key_lock);
 	return ret;
 }
@@ -346,6 +387,8 @@ int dp_hdcp_attach(struct dp_hdcp *hdcp, struct drm_connector *connector,
 		   struct drm_bridge *bridge, struct dp_catalog *catalog)
 {
 	struct drm_hdcp_helper_data *helper_data;
+	struct drm_device *dev;
+	struct drm_property *prop;
 
 	/* HDCP is not configured for this device */
 	if (!hdcp->parser->io.dp_controller.hdcp_key.base)
@@ -357,7 +400,20 @@ int dp_hdcp_attach(struct dp_hdcp *hdcp, struct drm_connector *connector,
 		return PTR_ERR(helper_data);
 
 	helper_data->driver_data = bridge;
-	hdcp->dev = connector->dev;
+
+	dev = connector->dev;
+	prop = dev->mode_config.content_protection_key_property;
+	if (!prop) {
+		prop = drm_property_create(dev, DRM_MODE_PROP_BLOB,
+					   "Content Protection Key", 0);
+	}
+	if (!prop)
+		return -1;
+	drm_object_attach_property(&connector->base, prop,
+				   DRM_MODE_HDCP_CONTENT_TYPE0);
+	dev->mode_config.content_protection_key_property = prop;
+
+	hdcp->dev = dev;
 	hdcp->connector = connector;
 	hdcp->helper_data = helper_data;
 	hdcp->catalog = catalog;
-- 
2.40.0.634.g4ca3ef3211-goog

Re: [PATCH v2 2/3] DRM: Create new Content Protection connector property

[Dmitry Baryshkov]

On 21/04/2023 19:27, Mark Yacoub wrote:
> From: Mark Yacoub <markyacoub@chromium.org>

Nit: is there a reason for this header? My first impression is that it 
matches your outgoing name & email address and as such is not necessary.

Nit#2: subject should mention 'Key', as you are creating a property for 
the key.

> 
> [Why]
> To enable Protected Content, some drivers require a key to be injected
> from user space to enable HDCP on the connector.
> 
> [How]
> Create new "Content Protection Property" of type "Blob"

Generic observation is that the ability to inject HDCP keys manually 
seems to be quite unique to your hardware. As such, I think the debugfs 
or sysfs suits better in comparison to the DRM property.

> 
> Signed-off-by: Mark Yacoub <markyacoub@chromium.org>
> ---
>   drivers/gpu/drm/drm_atomic_uapi.c | 9 +++++++++
>   include/drm/drm_connector.h       | 6 ++++++
>   include/drm/drm_mode_config.h     | 6 ++++++
>   3 files changed, 21 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> index d867e7f9f2cd5..e20bc57cdb05c 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -749,6 +749,11 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector,
>   		state->content_protection = val;
>   	} else if (property == config->hdcp_content_type_property) {
>   		state->hdcp_content_type = val;
> +	} else if (property == config->content_protection_key_property) {
> +		ret = drm_atomic_replace_property_blob_from_id(
> +			dev, &state->content_protection_key, val, -1, -1,
> +			&replaced);
> +		return ret;
>   	} else if (property == connector->colorspace_property) {
>   		state->colorspace = val;
>   	} else if (property == config->writeback_fb_id_property) {
> @@ -843,6 +848,10 @@ drm_atomic_connector_get_property(struct drm_connector *connector,
>   		*val = state->content_protection;
>   	} else if (property == config->hdcp_content_type_property) {
>   		*val = state->hdcp_content_type;
> +	} else if (property == config->content_protection_key_property) {
> +		*val = state->content_protection_key ?
> +			       state->content_protection_key->base.id :
> +			       0;
>   	} else if (property == config->writeback_fb_id_property) {
>   		/* Writeback framebuffer is one-shot, write and forget */
>   		*val = 0;
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 7b5048516185c..2fbe51272bfeb 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -896,6 +896,12 @@ struct drm_connector_state {
>   	 */
>   	unsigned int content_protection;
>   
> +	/**
> +	 * @content_protection_key: DRM blob property for holding the Content
> +	 * Protection Key injected from user space.
> +	 */
> +	struct drm_property_blob *content_protection_key;
> +
>   	/**
>   	 * @colorspace: State variable for Connector property to request
>   	 * colorspace change on Sink. This is most commonly used to switch
> diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
> index e5b053001d22e..615d1e5f57562 100644
> --- a/include/drm/drm_mode_config.h
> +++ b/include/drm/drm_mode_config.h
> @@ -887,6 +887,12 @@ struct drm_mode_config {
>   	 */
>   	struct drm_property *hdcp_content_type_property;
>   
> +	/**
> +	 * @content_protection_key_property: DRM blob property that receives the
> +	 * content protection key from user space to be injected into the kernel.
> +	 */
> +	struct drm_property *content_protection_key_property;
> +
>   	/* dumb ioctl parameters */
>   	uint32_t preferred_depth, prefer_shadow;
>   

-- 
With best wishes
Dmitry

Re: [PATCH v2 2/3] DRM: Create new Content Protection connector property

[Dave Stevenson]

Hi Mark (and Dmitry)

On Fri, 21 Apr 2023 at 18:07, Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On 21/04/2023 19:27, Mark Yacoub wrote:
> > From: Mark Yacoub <markyacoub@chromium.org>
>
> Nit: is there a reason for this header? My first impression is that it
> matches your outgoing name & email address and as such is not necessary.
>
> Nit#2: subject should mention 'Key', as you are creating a property for
> the key.
>
> >
> > [Why]
> > To enable Protected Content, some drivers require a key to be injected
> > from user space to enable HDCP on the connector.
> >
> > [How]
> > Create new "Content Protection Property" of type "Blob"
>
> Generic observation is that the ability to inject HDCP keys manually
> seems to be quite unique to your hardware. As such, I think the debugfs
> or sysfs suits better in comparison to the DRM property.

I was about to reply to v1 with a very similar comment over the
requirement to keep HDCP keys secret.

v2 has added WRITE_ONLY blobs so at least another process can't just
read the blob back out again, but it feels like there are still
numerous ways to grab those keys. For an unsecured userspace to have
the keys in the first place seems like a bad move, and IMHO they
should only be held in either a secure environment, or only held in
hardware (passed direct from OTP to HDCP block).


There's also the DRM uAPI requirement for having reviewed patches for
an open source project to go alongside any uAPI change. Do such
patches exist? https://docs.kernel.org/gpu/drm-uapi.html#open-source-userspace-requirements

  Dave

> >
> > Signed-off-by: Mark Yacoub <markyacoub@chromium.org>
> > ---
> >   drivers/gpu/drm/drm_atomic_uapi.c | 9 +++++++++
> >   include/drm/drm_connector.h       | 6 ++++++
> >   include/drm/drm_mode_config.h     | 6 ++++++
> >   3 files changed, 21 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> > index d867e7f9f2cd5..e20bc57cdb05c 100644
> > --- a/drivers/gpu/drm/drm_atomic_uapi.c
> > +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> > @@ -749,6 +749,11 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector,
> >               state->content_protection = val;
> >       } else if (property == config->hdcp_content_type_property) {
> >               state->hdcp_content_type = val;
> > +     } else if (property == config->content_protection_key_property) {
> > +             ret = drm_atomic_replace_property_blob_from_id(
> > +                     dev, &state->content_protection_key, val, -1, -1,
> > +                     &replaced);
> > +             return ret;
> >       } else if (property == connector->colorspace_property) {
> >               state->colorspace = val;
> >       } else if (property == config->writeback_fb_id_property) {
> > @@ -843,6 +848,10 @@ drm_atomic_connector_get_property(struct drm_connector *connector,
> >               *val = state->content_protection;
> >       } else if (property == config->hdcp_content_type_property) {
> >               *val = state->hdcp_content_type;
> > +     } else if (property == config->content_protection_key_property) {
> > +             *val = state->content_protection_key ?
> > +                            state->content_protection_key->base.id :
> > +                            0;
> >       } else if (property == config->writeback_fb_id_property) {
> >               /* Writeback framebuffer is one-shot, write and forget */
> >               *val = 0;
> > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> > index 7b5048516185c..2fbe51272bfeb 100644
> > --- a/include/drm/drm_connector.h
> > +++ b/include/drm/drm_connector.h
> > @@ -896,6 +896,12 @@ struct drm_connector_state {
> >        */
> >       unsigned int content_protection;
> >
> > +     /**
> > +      * @content_protection_key: DRM blob property for holding the Content
> > +      * Protection Key injected from user space.
> > +      */
> > +     struct drm_property_blob *content_protection_key;
> > +
> >       /**
> >        * @colorspace: State variable for Connector property to request
> >        * colorspace change on Sink. This is most commonly used to switch
> > diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
> > index e5b053001d22e..615d1e5f57562 100644
> > --- a/include/drm/drm_mode_config.h
> > +++ b/include/drm/drm_mode_config.h
> > @@ -887,6 +887,12 @@ struct drm_mode_config {
> >        */
> >       struct drm_property *hdcp_content_type_property;
> >
> > +     /**
> > +      * @content_protection_key_property: DRM blob property that receives the
> > +      * content protection key from user space to be injected into the kernel.
> > +      */
> > +     struct drm_property *content_protection_key_property;
> > +
> >       /* dumb ioctl parameters */
> >       uint32_t preferred_depth, prefer_shadow;
> >
>
> --
> With best wishes
> Dmitry
>

Re: [PATCH v2 1/3] drm: Create support for Write-Only property blob

[Daniel Vetter]

On Fri, Apr 21, 2023 at 12:27:47PM -0400, Mark Yacoub wrote:
> From: Mark Yacoub <markyacoub@chromium.org>
> 
> [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 <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 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

Re: [PATCH v2 1/3] drm: Create support for Write-Only property blob

[Sean Paul]

On Thu, Apr 27, 2023 at 5:59 AM Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Fri, Apr 21, 2023 at 12:27:47PM -0400, Mark Yacoub wrote:
> > From: Mark Yacoub <markyacoub@chromium.org>
> >
> > [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 <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 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