[PATCH v5 05/12] drm: revocation check at drm subsystem

Daniel Vetter daniel at ffwll.ch
Mon Apr 29 07:26:07 UTC 2019


On Thu, Apr 18, 2019 at 02:27:58PM +0530, Ramalingam C wrote:
> On every hdcp revocation check request SRM is read from fw file
> /lib/firmware/display_hdcp_srm.bin
> 
> SRM table is parsed and stored at drm_hdcp.c, with functions exported
> for the services for revocation check from drivers (which
> implements the HDCP authentication)
> 
> This patch handles the HDCP1.4 and 2.2 versions of SRM table.
> 
> v2:
>   moved the uAPI to request_firmware_direct() [Daniel]
> 
> Signed-off-by: Ramalingam C <ramalingam.c at intel.com>
> Suggested-by: Daniel Vetter <daniel at ffwll.ch>
> ---
>  drivers/gpu/drm/Makefile       |   2 +-
>  drivers/gpu/drm/drm_hdcp.c     | 336 +++++++++++++++++++++++++++++++++

Please add an include stanza for this new file to
Documentation/gpu/drm-kms-helpers.rst, somewhere near the other dp/hdcp
related sink helpers.

>  drivers/gpu/drm/drm_internal.h |   4 +
>  drivers/gpu/drm/drm_sysfs.c    |   2 +
>  include/drm/drm_hdcp.h         |  36 ++++
>  5 files changed, 379 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/gpu/drm/drm_hdcp.c
> 
> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> index 3d0c75cd687c..fe8400af2426 100644
> --- a/drivers/gpu/drm/Makefile
> +++ b/drivers/gpu/drm/Makefile
> @@ -19,7 +19,7 @@ drm-y       :=	drm_auth.o drm_bufs.o drm_cache.o \
>  		drm_plane.o drm_color_mgmt.o drm_print.o \
>  		drm_dumb_buffers.o drm_mode_config.o drm_vblank.o \
>  		drm_syncobj.o drm_lease.o drm_writeback.o drm_client.o \
> -		drm_atomic_uapi.o
> +		drm_atomic_uapi.o drm_hdcp.o
>  
>  drm-$(CONFIG_DRM_LIB_RANDOM) += lib/drm_random.o
>  drm-$(CONFIG_DRM_VM) += drm_vm.o
> diff --git a/drivers/gpu/drm/drm_hdcp.c b/drivers/gpu/drm/drm_hdcp.c
> new file mode 100644
> index 000000000000..78b043c8195e
> --- /dev/null
> +++ b/drivers/gpu/drm/drm_hdcp.c
> @@ -0,0 +1,336 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2019 Intel Corporation.
> + *
> + * Authors:
> + * Ramalingam C <ramalingam.c at intel.com>
> + */
> +
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/gfp.h>
> +#include <linux/export.h>
> +#include <linux/slab.h>
> +#include <linux/firmware.h>
> +
> +#include <drm/drm_hdcp.h>
> +#include <drm/drm_sysfs.h>
> +#include <drm/drm_print.h>
> +#include <drm/drm_device.h>
> +
> +struct hdcp_srm {
> +	u8 *srm_buf;
> +	size_t received_srm_sz;
> +	u32 revocated_ksv_cnt;
> +	u8 *revocated_ksv_list;
> +
> +	/* Mutex to protect above struct member */
> +	struct mutex mutex;
> +} *srm_data;
> +
> +static inline void drm_hdcp_print_ksv(const char *ksv)
> +{
> +	DRM_DEBUG("\t%#04x, %#04x, %#04x, %#04x, %#04x\n", *ksv & 0xff,

Why 04? 8 bit only needs 02 I think.

> +		  *(ksv + 1) & 0xff, *(ksv + 2) & 0xff, *(ksv + 3) & 0xff,
> +		  *(ksv + 4) & 0xff);

The 0xff is redundant, char is always only 8 bits. You could also simplify
the array derefe using ksv[0], ksv[1], ... it's the same expression.

Another one: There's a bit a confusion between const char * and u8* for
parsing the buffer. I think it'd be good to standardize on const u8* for
everything. I think that should also remove the need for 0xff completely,
because no more sign extensions to the full int.

> +}
> +
> +static u32 drm_hdcp_get_revocated_ksv_count(const char *buf, u32 vrls_length)
> +{
> +	u32 parsed_bytes = 0, ksv_count = 0, vrl_ksv_cnt, vrl_sz;
> +
> +	do {
> +		vrl_ksv_cnt = *buf;
> +		ksv_count += vrl_ksv_cnt;
> +
> +		vrl_sz = (vrl_ksv_cnt * DRM_HDCP_KSV_LEN) + 1;
> +		buf += vrl_sz;
> +		parsed_bytes += vrl_sz;
> +	} while (parsed_bytes < vrls_length);

Hm, if we overflow here (i.e. parsed_bytes > vrls_lenght) then we return
garbage, since we already incremented ksv_count. Plus there's no error
checking of this. Kernel shouldn't trust root this much, and if the
vrls_length and our ksv parsing don't agree, we should abort the srm load.
So maybe switch the return value to int, and on error return -EINVAL and
abort?

> +
> +	return ksv_count;
> +}
> +
> +static u32 drm_hdcp_get_revocated_ksvs(const char *buf, u8 *revocated_ksv_list,
> +				       u32 vrls_length)

This function here doesn't need to be paranoid, since we already checked
the SRM by this point.

> +{
> +	u32 parsed_bytes = 0, ksv_count = 0;
> +	u32 vrl_ksv_cnt, vrl_ksv_sz, vrl_idx = 0;
> +
> +	do {
> +		vrl_ksv_cnt = *buf;
> +		vrl_ksv_sz = vrl_ksv_cnt * DRM_HDCP_KSV_LEN;
> +
> +		buf++;
> +
> +		DRM_DEBUG("vrl: %d, Revoked KSVs: %d\n", vrl_idx++,
> +			  vrl_ksv_cnt);
> +		memcpy(revocated_ksv_list, buf, vrl_ksv_sz);
> +
> +		ksv_count += vrl_ksv_cnt;
> +		revocated_ksv_list += vrl_ksv_sz;
> +		buf += vrl_ksv_sz;
> +
> +		parsed_bytes += (vrl_ksv_sz + 1);
> +	} while (parsed_bytes < vrls_length);
> +
> +	return ksv_count;
> +}
> +
> +static int drm_hdcp_parse_hdcp1_srm(const char *buf, size_t count)
> +{
> +	struct hdcp_srm_header *header;
> +	u32 vrl_length, ksv_count;
> +
> +	if (count < (sizeof(struct hdcp_srm_header) +
> +	    DRM_HDCP_1_4_VRL_LENGTH_SIZE + DRM_HDCP_1_4_DCP_SIG_SIZE)) {
> +		DRM_ERROR("Invalid blob length\n");
> +		return -EINVAL;
> +	}
> +
> +	header = (struct hdcp_srm_header *)buf;
> +	mutex_lock(&srm_data->mutex);
> +	DRM_DEBUG("SRM ID: 0x%x, SRM Ver: 0x%x, SRM Gen No: 0x%x\n",
> +		  header->spec_indicator.srm_id,
> +		  __swab16(header->srm_version), header->srm_gen_no);
> +
> +	WARN_ON(header->spec_indicator.reserved_hi ||
> +		header->spec_indicator.reserved_lo);
> +
> +	if (header->spec_indicator.srm_id != DRM_HDCP_1_4_SRM_ID) {
> +		DRM_ERROR("Invalid srm_id\n");
> +		mutex_unlock(&srm_data->mutex);
> +		return -EINVAL;
> +	}
> +
> +	buf = buf + sizeof(*header);
> +	vrl_length = (*buf << 16 | *(buf + 1) << 8 | *(buf + 2));

Maybe a static inline to implement this and use in the hdcp2 version below
too. Also I think buf[0], buf[1] is easier to read (as a bikeshed again).

> +	if (count < (sizeof(struct hdcp_srm_header) + vrl_length) ||
> +	    vrl_length < (DRM_HDCP_1_4_VRL_LENGTH_SIZE +
> +			  DRM_HDCP_1_4_DCP_SIG_SIZE)) {
> +		DRM_ERROR("Invalid blob length or vrl length\n");
> +		mutex_unlock(&srm_data->mutex);
> +		return -EINVAL;
> +	}
> +
> +	/* Length of the all vrls combined */
> +	vrl_length -= (DRM_HDCP_1_4_VRL_LENGTH_SIZE +
> +		       DRM_HDCP_1_4_DCP_SIG_SIZE);
> +
> +	if (!vrl_length) {
> +		DRM_ERROR("No vrl found\n");
> +		mutex_unlock(&srm_data->mutex);
> +		return -EINVAL;
> +	}
> +
> +	buf += DRM_HDCP_1_4_VRL_LENGTH_SIZE;
> +	ksv_count = drm_hdcp_get_revocated_ksv_count(buf, vrl_length);
> +	if (!ksv_count) {
> +		DRM_DEBUG("Revocated KSV count is 0\n");
> +		mutex_unlock(&srm_data->mutex);
> +		return count;
> +	}
> +
> +	kfree(srm_data->revocated_ksv_list);
> +	srm_data->revocated_ksv_list = kzalloc(ksv_count * DRM_HDCP_KSV_LEN,
> +					       GFP_KERNEL);

kcalloc()

> +	if (!srm_data->revocated_ksv_list) {
> +		DRM_ERROR("Out of Memory\n");
> +		mutex_unlock(&srm_data->mutex);
> +		return -ENOMEM;
> +	}
> +
> +	if (drm_hdcp_get_revocated_ksvs(buf, srm_data->revocated_ksv_list,
> +					vrl_length) != ksv_count) {
> +		srm_data->revocated_ksv_cnt = 0;
> +		kfree(srm_data->revocated_ksv_list);
> +		mutex_unlock(&srm_data->mutex);
> +		return -EINVAL;
> +	}
> +
> +	srm_data->revocated_ksv_cnt = ksv_count;
> +	mutex_unlock(&srm_data->mutex);
> +	return count;
> +}
> +
> +static int drm_hdcp_parse_hdcp2_srm(const char *buf, size_t count)
> +{
> +	struct hdcp2_srm_header *header;
> +	u32 vrl_length, ksv_count, ksv_sz;
> +
> +	mutex_lock(&srm_data->mutex);
> +	if (count < (sizeof(struct hdcp2_srm_header) +
> +	    DRM_HDCP_2_VRL_LENGTH_SIZE + DRM_HDCP_2_DCP_SIG_SIZE)) {
> +		DRM_ERROR("Invalid blob length\n");
> +		mutex_unlock(&srm_data->mutex);
> +		return -EINVAL;
> +	}
> +
> +	header = (struct hdcp2_srm_header *)buf;
> +	DRM_DEBUG("SRM ID: 0x%x, SRM Ver: 0x%x, SRM Gen No: 0x%x\n",
> +		  header->spec_indicator.srm_id,
> +		  __swab16(header->srm_version), header->srm_gen_no);
> +
> +	if (header->spec_indicator.reserved)
> +		return -EINVAL;
> +
> +	buf = buf + sizeof(*header);
> +	vrl_length = (*buf << 16 | *(buf + 1) << 8 | *(buf + 2));
> +
> +	if (count < (sizeof(struct hdcp2_srm_header) + vrl_length) ||
> +	    vrl_length < (DRM_HDCP_2_VRL_LENGTH_SIZE +
> +	    DRM_HDCP_2_DCP_SIG_SIZE)) {
> +		DRM_ERROR("Invalid blob length or vrl length\n");
> +		mutex_unlock(&srm_data->mutex);
> +		return -EINVAL;
> +	}
> +
> +	/* Length of the all vrls combined */
> +	vrl_length -= (DRM_HDCP_2_VRL_LENGTH_SIZE +
> +		       DRM_HDCP_2_DCP_SIG_SIZE);
> +
> +	if (!vrl_length) {
> +		DRM_ERROR("No vrl found\n");
> +		mutex_unlock(&srm_data->mutex);
> +		return -EINVAL;
> +	}
> +
> +	buf += DRM_HDCP_2_VRL_LENGTH_SIZE;
> +	ksv_count = (*buf << 2) | DRM_HDCP_2_KSV_COUNT_2_LSBITS(*(buf + 1));
> +	if (!ksv_count) {
> +		DRM_DEBUG("Revocated KSV count is 0\n");
> +		mutex_unlock(&srm_data->mutex);
> +		return count;
> +	}
> +
> +	kfree(srm_data->revocated_ksv_list);
> +	srm_data->revocated_ksv_list = kzalloc(ksv_count * DRM_HDCP_KSV_LEN,
> +					       GFP_KERNEL);
> +	if (!srm_data->revocated_ksv_list) {
> +		DRM_ERROR("Out of Memory\n");
> +		mutex_unlock(&srm_data->mutex);
> +		return -ENOMEM;
> +	}
> +
> +	ksv_sz = ksv_count * DRM_HDCP_KSV_LEN;
> +	buf += DRM_HDCP_2_NO_OF_DEV_PLUS_RESERVED_SZ;
> +
> +	DRM_DEBUG("Revoked KSVs: %d\n", ksv_count);
> +	memcpy(srm_data->revocated_ksv_list, buf, ksv_sz);
> +
> +	srm_data->revocated_ksv_cnt = ksv_count;
> +	mutex_unlock(&srm_data->mutex);
> +	return count;
> +}
> +
> +static inline bool is_srm_version_hdcp1(const char *buf)
> +{
> +	return ((u8)*buf) == DRM_HDCP_1_4_SRM_ID << 4;
> +}
> +
> +static inline bool is_srm_version_hdcp2(const char *buf)
> +{
> +	return ((u8)*buf) == (DRM_HDCP_2_SRM_ID << 4 |
> +			     DRM_HDCP_2_INDICATOR);
> +}
> +
> +static ssize_t drm_hdcp_srm_update(const char *buf, size_t count)
> +{
> +	if (is_srm_version_hdcp1(buf))
> +		return (ssize_t)drm_hdcp_parse_hdcp1_srm(buf, count);
> +	else if (is_srm_version_hdcp2(buf))
> +		return (ssize_t)drm_hdcp_parse_hdcp2_srm(buf, count);
> +
> +	return (ssize_t)-EINVAL;
> +}
> +
> +void drm_hdcp_request_srm(struct drm_device *drm_dev)
> +{
> +	char fw_name[36] = "display_hdcp_srm.bin";
> +	const struct firmware *fw;
> +
> +	int ret;
> +
> +	ret = request_firmware_direct(&fw, (const char *)fw_name,
> +				      drm_dev->dev);

We need an ack from Matt Roper or someont else from iotg on this.

> +	if (ret < 0)
> +		goto exit;
> +
> +	if (fw->size && fw->data)
> +		drm_hdcp_srm_update((const char *)fw->data, fw->size);
> +
> +exit:
> +	release_firmware(fw);
> +}
> +
> +/* Check if any of the KSV is revocated by DCP LLC through SRM table */
> +bool drm_hdcp_ksvs_revocated(struct drm_device *drm_dev, u8 *ksvs,

Since comment says "check", maybe put that into the function name? I like
to have verbs in function names that do something (instead of simple
helper functions to extract a computation to make the code a bit more
readable).

Also needs some nice kerneldoc.

> +			     u32 ksv_count)
> +{
> +	u32 rev_ksv_cnt, cnt, i, j;
> +	u8 *rev_ksv_list;
> +
> +	if (!srm_data)
> +		return false;
> +
> +	drm_hdcp_request_srm(drm_dev);
> +
> +	mutex_lock(&srm_data->mutex);

I'd have wrapped the locking around the entire function, should simplify
things a lot. Or does that anger lockdep when we call into
request_firmware()?

> +	rev_ksv_cnt = srm_data->revocated_ksv_cnt;
> +	rev_ksv_list = srm_data->revocated_ksv_list;
> +
> +	/* If the Revocated ksv list is empty */
> +	if (!rev_ksv_cnt || !rev_ksv_list) {
> +		mutex_unlock(&srm_data->mutex);
> +		return false;
> +	}
> +
> +	for  (cnt = 0; cnt < ksv_count; cnt++) {
> +		rev_ksv_list = srm_data->revocated_ksv_list;
> +		for (i = 0; i < rev_ksv_cnt; i++) {
> +			for (j = 0; j < DRM_HDCP_KSV_LEN; j++)
> +				if (*(ksvs + j) != *(rev_ksv_list + j)) {
> +					break;
> +				} else if (j == (DRM_HDCP_KSV_LEN - 1)) {
> +					DRM_DEBUG("Revocated KSV is ");
> +					drm_hdcp_print_ksv(ksvs);
> +					mutex_unlock(&srm_data->mutex);
> +					return true;
> +				}
> +			/* Move the offset to next KSV in the revocated list */
> +			rev_ksv_list += DRM_HDCP_KSV_LEN;
> +		}
> +
> +		/* Iterate to next ksv_offset */
> +		ksvs += DRM_HDCP_KSV_LEN;
> +	}
> +	mutex_unlock(&srm_data->mutex);
> +	return false;
> +}
> +EXPORT_SYMBOL_GPL(drm_hdcp_ksvs_revocated);
> +
> +int drm_setup_hdcp_srm(struct class *drm_class)
> +{
> +	srm_data = kzalloc(sizeof(*srm_data), GFP_KERNEL);
> +	if (!srm_data)
> +		return -ENOMEM;
> +
> +	srm_data->srm_buf = kcalloc(DRM_HDCP_SRM_GEN1_MAX_BYTES,
> +				    sizeof(u8), GFP_KERNEL);
> +	if (!srm_data->srm_buf) {
> +		kfree(srm_data);
> +		return -ENOMEM;
> +	}
> +	mutex_init(&srm_data->mutex);
> +
> +	return 0;
> +}
> +
> +void drm_teardown_hdcp_srm(struct class *drm_class)
> +{
> +	if (srm_data) {
> +		kfree(srm_data->srm_buf);
> +		kfree(srm_data->revocated_ksv_list);
> +		kfree(srm_data);
> +	}
> +}
> diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
> index d9a483a5fce0..ef3180443870 100644
> --- a/drivers/gpu/drm/drm_internal.h
> +++ b/drivers/gpu/drm/drm_internal.h
> @@ -199,3 +199,7 @@ int drm_syncobj_query_ioctl(struct drm_device *dev, void *data,
>  void drm_framebuffer_print_info(struct drm_printer *p, unsigned int indent,
>  				const struct drm_framebuffer *fb);
>  int drm_framebuffer_debugfs_init(struct drm_minor *minor);
> +
> +/* drm_hdcp.c */
> +int drm_setup_hdcp_srm(struct class *drm_class);
> +void drm_teardown_hdcp_srm(struct class *drm_class);
> diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
> index ecb7b33002bb..18b1ac442997 100644
> --- a/drivers/gpu/drm/drm_sysfs.c
> +++ b/drivers/gpu/drm/drm_sysfs.c
> @@ -78,6 +78,7 @@ int drm_sysfs_init(void)
>  	}
>  
>  	drm_class->devnode = drm_devnode;
> +	drm_setup_hdcp_srm(drm_class);
>  	return 0;
>  }
>  
> @@ -90,6 +91,7 @@ void drm_sysfs_destroy(void)
>  {
>  	if (IS_ERR_OR_NULL(drm_class))
>  		return;
> +	drm_teardown_hdcp_srm(drm_class);
>  	class_remove_file(drm_class, &class_attr_version.attr);
>  	class_destroy(drm_class);
>  	drm_class = NULL;
> diff --git a/include/drm/drm_hdcp.h b/include/drm/drm_hdcp.h
> index f243408ecf26..ff2bcfc1ecef 100644
> --- a/include/drm/drm_hdcp.h
> +++ b/include/drm/drm_hdcp.h
> @@ -265,4 +265,40 @@ void drm_hdcp2_u32_to_seq_num(u8 seq_num[HDCP_2_2_SEQ_NUM_LEN], u32 val)
>  	seq_num[2] = val;
>  }
>  
> +#define DRM_HDCP_SRM_GEN1_MAX_BYTES		(5 * 1024)
> +#define DRM_HDCP_1_4_SRM_ID			0x8
> +#define DRM_HDCP_1_4_VRL_LENGTH_SIZE		3
> +#define DRM_HDCP_1_4_DCP_SIG_SIZE		40
> +
> +struct hdcp_srm_header {
> +	struct {
> +		u8 reserved_hi:4;
> +		u8 srm_id:4;

bitfields in binary structures are heavily discouraged. I think since this
is only u8 it should be fine, but explaining that is more work than just
having the usual _MASK/_SHIFT #defines like we do with registers.

> +		u8 reserved_lo;
> +	} spec_indicator;

If you wannt __packed, you also need to list this to sub-structs.

> +	u16 srm_version;
> +	u8 srm_gen_no;
> +} __packed;
> +
> +#define DRM_HDCP_2_SRM_ID			0x9
> +#define DRM_HDCP_2_INDICATOR			0x1
> +#define DRM_HDCP_2_VRL_LENGTH_SIZE		3
> +#define DRM_HDCP_2_DCP_SIG_SIZE			384
> +#define DRM_HDCP_2_NO_OF_DEV_PLUS_RESERVED_SZ	4
> +
> +#define DRM_HDCP_2_KSV_COUNT_2_LSBITS(byte)	(((byte) & 0xC) >> 6)
> +
> +struct hdcp2_srm_header {
> +	struct {
> +		u8 hdcp2_indicator:4;

Since hdcp1/2 match except for this I think better to merge them.

> +		u8 srm_id:4;
> +		u8 reserved;
> +	} spec_indicator;
> +	u16 srm_version;
> +	u8 srm_gen_no;
> +} __packed;
> +
> +struct drm_device;
> +
> +bool drm_hdcp_ksvs_revocated(struct drm_device *dev, u8 *ksvs, u32 ksv_count);
>  #endif

Cheers, Daniel

> -- 
> 2.19.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch


More information about the dri-devel mailing list