Re: [PATCH V1 1/2] soc: qcom: smem: validate fields of shared structures

From: Bjorn Andersson
Date: Wed Jun 23 2021 - 13:25:49 EST

Next message: Rafael J. Wysocki: "Re: [PATCH] software node: Handle software node injection to an existing device properly"
Previous message: Greg KH: "Re: [PATCH 4/4] drivers: net: mediatek: initial implementation of ccmni"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Tue 09 Jun 06:40 CDT 2020, Deepak Kumar Singh wrote:

> Structures in shared memory that can be modified by remote
> processors may have untrusted values, they should be validated
> before use.
>
> Adding proper validation before using fields of shared
> structures.
>
> Signed-off-by: Deepak Kumar Singh <deesin@xxxxxxxxxxxxxx>
> ---
> drivers/soc/qcom/smem.c | 194 +++++++++++++++++++++++++++++++++---------------
> 1 file changed, 133 insertions(+), 61 deletions(-)
>
> diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
> index 28c19bc..c1bd310 100644
> --- a/drivers/soc/qcom/smem.c
> +++ b/drivers/soc/qcom/smem.c
> @@ -249,11 +249,9 @@ struct smem_region {
> * struct qcom_smem - device data for the smem device
> * @dev: device pointer
> * @hwlock: reference to a hwspinlock
> - * @global_partition: pointer to global partition when in use
> - * @global_cacheline: cacheline size for global partition
> - * @partitions: list of pointers to partitions affecting the current
> + * @global_partition_entry: pointer to global partition entry when in use
> + * @ptable_entries: list of pointers to partitions table entry of current
> * processor/host
> - * @cacheline: list of cacheline sizes for each host
> * @item_count: max accepted item number
> * @num_regions: number of @regions
> * @regions: list of the memory regions defining the shared memory
> @@ -263,10 +261,8 @@ struct qcom_smem {
>
> struct hwspinlock *hwlock;
>
> - struct smem_partition_header *global_partition;
> - size_t global_cacheline;
> - struct smem_partition_header *partitions[SMEM_HOST_COUNT];
> - size_t cacheline[SMEM_HOST_COUNT];
> + struct smem_ptable_entry *global_partition_entry;
> + struct smem_ptable_entry *ptable_entries[SMEM_HOST_COUNT];

Could you please split this patch in one that transitions these to
smem_ptable_entry and then a separate one that adds the new range
checks.

> u32 item_count;
> struct platform_device *socinfo;
>
> @@ -274,7 +270,19 @@ struct qcom_smem {
> struct smem_region regions[];
> };
>
> -static void *
> +/* Pointer to the one and only smem handle */
> +static struct qcom_smem *__smem;
> +
> +/* Timeout (ms) for the trylock of remote spinlocks */
> +#define HWSPINLOCK_TIMEOUT 1000
> +
> +static struct smem_partition_header *
> +ptable_entry_to_phdr(struct smem_ptable_entry *entry)
> +{
> + return __smem->regions[0].virt_base + le32_to_cpu(entry->offset);

Overall __smem was only used to acquire the struct qcom_smem handle, but
after that we pass that pointer around - rather than operating on the
global variable.

So how about passing the struct qcom_smem as a first argument to this
function, to stick to this kind of design?

> +}
> +
> +static struct smem_private_entry *
> phdr_to_last_uncached_entry(struct smem_partition_header *phdr)
> {
> void *p = phdr;
> @@ -339,25 +347,27 @@ static void *cached_entry_to_item(struct smem_private_entry *e)
> return p - le32_to_cpu(e->size);
> }
>
> -/* Pointer to the one and only smem handle */
> -static struct qcom_smem *__smem;
> -
> -/* Timeout (ms) for the trylock of remote spinlocks */
> -#define HWSPINLOCK_TIMEOUT 1000

This should have the side effect of you being able to leave there where
they were.

> -
> static int qcom_smem_alloc_private(struct qcom_smem *smem,
> - struct smem_partition_header *phdr,
> + struct smem_ptable_entry *entry,
> unsigned item,
> size_t size)
> {
> struct smem_private_entry *hdr, *end;
> + struct smem_partition_header *phdr;
> size_t alloc_size;
> void *cached;
> + void *p_end;
> +
> + phdr = ptable_entry_to_phdr(entry);
> + p_end = (void *)phdr + le32_to_cpu(entry->size);
>
> hdr = phdr_to_first_uncached_entry(phdr);
> end = phdr_to_last_uncached_entry(phdr);
> cached = phdr_to_last_cached_entry(phdr);
>
> + if (WARN_ON((void *)end > p_end || (void *)cached > p_end))
> + return -EINVAL;
> +
> while (hdr < end) {
> if (hdr->canary != SMEM_PRIVATE_CANARY)
> goto bad_canary;
> @@ -366,6 +376,8 @@ static int qcom_smem_alloc_private(struct qcom_smem *smem,
>
> hdr = uncached_entry_next(hdr);
> }
> + if (WARN_ON((void *)hdr > p_end))
> + return -EINVAL;
>
> /* Check that we don't grow into the cached region */
> alloc_size = sizeof(*hdr) + ALIGN(size, 8);
> @@ -440,7 +452,7 @@ static int qcom_smem_alloc_global(struct qcom_smem *smem,
> */
> int qcom_smem_alloc(unsigned host, unsigned item, size_t size)
> {
> - struct smem_partition_header *phdr;
> + struct smem_ptable_entry *entry;
> unsigned long flags;
> int ret;
>
> @@ -462,12 +474,12 @@ int qcom_smem_alloc(unsigned host, unsigned item, size_t size)
> if (ret)
> return ret;
>
> - if (host < SMEM_HOST_COUNT && __smem->partitions[host]) {
> - phdr = __smem->partitions[host];
> - ret = qcom_smem_alloc_private(__smem, phdr, item, size);
> - } else if (__smem->global_partition) {
> - phdr = __smem->global_partition;
> - ret = qcom_smem_alloc_private(__smem, phdr, item, size);
> + if (host < SMEM_HOST_COUNT && __smem->ptable_entries[host]) {
> + entry = __smem->ptable_entries[host];
> + ret = qcom_smem_alloc_private(__smem, entry, item, size);
> + } else if (__smem->global_partition_entry) {
> + entry = __smem->global_partition_entry;
> + ret = qcom_smem_alloc_private(__smem, entry, item, size);
> } else {
> ret = qcom_smem_alloc_global(__smem, item, size);
> }
> @@ -482,9 +494,11 @@ static void *qcom_smem_get_global(struct qcom_smem *smem,
> unsigned item,
> size_t *size)
> {
> - struct smem_header *header;
> - struct smem_region *region;
> struct smem_global_entry *entry;
> + struct smem_header *header;
> + struct smem_region *area;

Perhaps leave this as "region" and keep "header" the first, to limit
this function to only the logical changes?

> + u64 entry_offset;
> + u32 e_size;
> u32 aux_base;
> unsigned i;
>
> @@ -496,12 +510,19 @@ static void *qcom_smem_get_global(struct qcom_smem *smem,
> aux_base = le32_to_cpu(entry->aux_base) & AUX_BASE_MASK;
>
> for (i = 0; i < smem->num_regions; i++) {
> - region = &smem->regions[i];
> + area = &smem->regions[i];
> +
> + if (area->aux_base == aux_base || !aux_base) {
> + e_size = le32_to_cpu(entry->size);
> + entry_offset = le32_to_cpu(entry->offset);
> +
> + if (WARN_ON(e_size + entry_offset > area->size))
> + return ERR_PTR(-EINVAL);
>
> - if (region->aux_base == aux_base || !aux_base) {
> if (size != NULL)
> - *size = le32_to_cpu(entry->size);
> - return region->virt_base + le32_to_cpu(entry->offset);
> + *size = e_size;
> +
> + return area->virt_base + entry_offset;
> }
> }
>
> @@ -509,50 +530,92 @@ static void *qcom_smem_get_global(struct qcom_smem *smem,
> }
>
> static void *qcom_smem_get_private(struct qcom_smem *smem,
> - struct smem_partition_header *phdr,
> - size_t cacheline,
> + struct smem_ptable_entry *entry,
> unsigned item,
> size_t *size)
> {
> struct smem_private_entry *e, *end;
> + struct smem_partition_header *phdr;
> + void *item_ptr, *p_end;
> + u32 partition_size;
> + size_t cacheline;
> + u32 padding_data;
> + u32 e_size;
> +
> + phdr = ptable_entry_to_phdr(entry);
> + partition_size = le32_to_cpu(entry->size);
> + p_end = (void *)phdr + partition_size;
> + cacheline = le32_to_cpu(entry->cacheline);
>
> e = phdr_to_first_uncached_entry(phdr);
> end = phdr_to_last_uncached_entry(phdr);
>
> + if (WARN_ON((void *)end > p_end))
> + return ERR_PTR(-EINVAL);
> +
> while (e < end) {
> if (e->canary != SMEM_PRIVATE_CANARY)
> goto invalid_canary;
>
> if (le16_to_cpu(e->item) == item) {
> - if (size != NULL)
> - *size = le32_to_cpu(e->size) -
> - le16_to_cpu(e->padding_data);
> -
> - return uncached_entry_to_item(e);
> + if (size != NULL) {
> + e_size = le32_to_cpu(e->size);
> + padding_data = le16_to_cpu(e->padding_data);
> +
> + if (e_size < partition_size
> + && padding_data < e_size)

It's fine to unwrap this line and go over 80 chars when it makes the
code easier to read.

> + *size = e_size - padding_data;
> + else
> + return ERR_PTR(-EINVAL);

Flip this around and keep doing:

if (WARN_ON(invalid))
return - EINVAL;

continue with the good stuff;

> + }
> +
> + item_ptr = uncached_entry_to_item(e);

There's two spaces after the '='

> + if (WARN_ON(item_ptr > p_end))
> + return ERR_PTR(-EINVAL);
> +
> + return item_ptr;
> }
>
> e = uncached_entry_next(e);
> }
> + if (WARN_ON((void *)e > p_end))
> + return ERR_PTR(-EINVAL);
>
> /* Item was not found in the uncached list, search the cached list */
>
> e = phdr_to_first_cached_entry(phdr, cacheline);
> end = phdr_to_last_cached_entry(phdr);
>
> + if (WARN_ON((void *)e < (void *)phdr || (void *)end > p_end))
> + return ERR_PTR(-EINVAL);
> +
> while (e > end) {
> if (e->canary != SMEM_PRIVATE_CANARY)
> goto invalid_canary;
>
> if (le16_to_cpu(e->item) == item) {
> - if (size != NULL)
> - *size = le32_to_cpu(e->size) -
> - le16_to_cpu(e->padding_data);
> -
> - return cached_entry_to_item(e);
> + if (size != NULL) {
> + e_size = le32_to_cpu(e->size);
> + padding_data = le16_to_cpu(e->padding_data);
> +
> + if (e_size < partition_size
> + && padding_data < e_size)
> + *size = e_size - padding_data;
> + else
> + return ERR_PTR(-EINVAL);

Same as above.

Regards,
Bjorn

> + }
> +
> + item_ptr = cached_entry_to_item(e);
> + if (WARN_ON(item_ptr < (void *)phdr))
> + return ERR_PTR(-EINVAL);
> +
> + return item_ptr;
> }
>
> e = cached_entry_next(e, cacheline);
> }
> + if (WARN_ON((void *)e < (void *)phdr))
> + return ERR_PTR(-EINVAL);
>
> return ERR_PTR(-ENOENT);
>
> @@ -574,9 +637,8 @@ static void *qcom_smem_get_private(struct qcom_smem *smem,
> */
> void *qcom_smem_get(unsigned host, unsigned item, size_t *size)
> {
> - struct smem_partition_header *phdr;
> + struct smem_ptable_entry *entry;
> unsigned long flags;
> - size_t cacheln;
> int ret;
> void *ptr = ERR_PTR(-EPROBE_DEFER);
>
> @@ -592,14 +654,12 @@ void *qcom_smem_get(unsigned host, unsigned item, size_t *size)
> if (ret)
> return ERR_PTR(ret);
>
> - if (host < SMEM_HOST_COUNT && __smem->partitions[host]) {
> - phdr = __smem->partitions[host];
> - cacheln = __smem->cacheline[host];
> - ptr = qcom_smem_get_private(__smem, phdr, cacheln, item, size);
> - } else if (__smem->global_partition) {
> - phdr = __smem->global_partition;
> - cacheln = __smem->global_cacheline;
> - ptr = qcom_smem_get_private(__smem, phdr, cacheln, item, size);
> + if (host < SMEM_HOST_COUNT && __smem->ptable_entries[host]) {
> + entry = __smem->ptable_entries[host];
> + ptr = qcom_smem_get_private(__smem, entry, item, size);
> + } else if (__smem->global_partition_entry) {
> + entry = __smem->global_partition_entry;
> + ptr = qcom_smem_get_private(__smem, entry, item, size);
> } else {
> ptr = qcom_smem_get_global(__smem, item, size);
> }
> @@ -621,23 +681,37 @@ EXPORT_SYMBOL(qcom_smem_get);
> int qcom_smem_get_free_space(unsigned host)
> {
> struct smem_partition_header *phdr;
> + struct smem_ptable_entry *entry;
> struct smem_header *header;
> unsigned ret;
>
> if (!__smem)
> return -EPROBE_DEFER;
>
> - if (host < SMEM_HOST_COUNT && __smem->partitions[host]) {
> - phdr = __smem->partitions[host];
> + if (host < SMEM_HOST_COUNT && __smem->ptable_entries[host]) {
> + entry = __smem->ptable_entries[host];
> + phdr = ptable_entry_to_phdr(entry);
> +
> ret = le32_to_cpu(phdr->offset_free_cached) -
> le32_to_cpu(phdr->offset_free_uncached);
> - } else if (__smem->global_partition) {
> - phdr = __smem->global_partition;
> +
> + if (ret > le32_to_cpu(entry->size))
> + return -EINVAL;
> + } else if (__smem->global_partition_entry) {
> + entry = __smem->global_partition_entry;
> + phdr = ptable_entry_to_phdr(entry);
> +
> ret = le32_to_cpu(phdr->offset_free_cached) -
> le32_to_cpu(phdr->offset_free_uncached);
> +
> + if (ret > le32_to_cpu(entry->size))
> + return -EINVAL;
> } else {
> header = __smem->regions[0].virt_base;
> ret = le32_to_cpu(header->available);
> +
> + if (ret > __smem->regions[0].size)
> + return -EINVAL;
> }
>
> return ret;
> @@ -772,7 +846,7 @@ static int qcom_smem_set_global_partition(struct qcom_smem *smem)
> bool found = false;
> int i;
>
> - if (smem->global_partition) {
> + if (smem->global_partition_entry) {
> dev_err(smem->dev, "Already found the global partition\n");
> return -EINVAL;
> }
> @@ -807,8 +881,7 @@ static int qcom_smem_set_global_partition(struct qcom_smem *smem)
> if (!header)
> return -EINVAL;
>
> - smem->global_partition = header;
> - smem->global_cacheline = le32_to_cpu(entry->cacheline);
> + smem->global_partition_entry = entry;
>
> return 0;
> }
> @@ -848,7 +921,7 @@ qcom_smem_enumerate_partitions(struct qcom_smem *smem, u16 local_host)
> return -EINVAL;
> }
>
> - if (smem->partitions[remote_host]) {
> + if (smem->ptable_entries[remote_host]) {
> dev_err(smem->dev, "duplicate host %hu\n", remote_host);
> return -EINVAL;
> }
> @@ -857,8 +930,7 @@ qcom_smem_enumerate_partitions(struct qcom_smem *smem, u16 local_host)
> if (!header)
> return -EINVAL;
>
> - smem->partitions[remote_host] = header;
> - smem->cacheline[remote_host] = le32_to_cpu(entry->cacheline);
> + smem->ptable_entries[remote_host] = entry;
> }
>
> return 0;
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>

Next message: Rafael J. Wysocki: "Re: [PATCH] software node: Handle software node injection to an existing device properly"
Previous message: Greg KH: "Re: [PATCH 4/4] drivers: net: mediatek: initial implementation of ccmni"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]