On Mon 14 Feb 06:46 PST 2022, 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.
I'm not able to find patch 1/2, did you send it out or is it just me
being unlucky finding it?
>
> Signed-off-by: Deepak Kumar Singh <quic_deesin@xxxxxxxxxxx>
> ---
> drivers/soc/qcom/smem.c | 81 +++++++++++++++++++++++++++++++++++++++++--------
> 1 file changed, 68 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
> index 96444ff..644844b 100644
> --- a/drivers/soc/qcom/smem.c
> +++ b/drivers/soc/qcom/smem.c
> @@ -367,13 +367,18 @@ static int qcom_smem_alloc_private(struct qcom_smem *smem,
> struct smem_partition_header *phdr;
> size_t alloc_size;
> void *cached;
> + void *p_end;
>
> phdr = (struct smem_partition_header __force *)part->virt_base;
> + p_end = (void *)phdr + part->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))
cached is a void * already, do you really need to cast it?
> + return -EINVAL;
> +
> while (hdr < end) {
> if (hdr->canary != SMEM_PRIVATE_CANARY)
> goto bad_canary;
> @@ -383,6 +388,9 @@ 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);
> if ((void *)hdr + alloc_size > cached) {
> @@ -501,6 +509,8 @@ static void *qcom_smem_get_global(struct qcom_smem *smem,
> struct smem_header *header;
> struct smem_region *region;
> struct smem_global_entry *entry;
> + u64 entry_offset;
> + u32 e_size;
> u32 aux_base;
> unsigned i;
>
> @@ -515,9 +525,13 @@ static void *qcom_smem_get_global(struct qcom_smem *smem,
> region = &smem->regions[i];
>
> if ((u32)region->aux_base == aux_base || !aux_base) {
> + e_size = le32_to_cpu(entry->size);
> + entry_offset = le32_to_cpu(entry->offset);
> +
> if (size != NULL)
> - *size = le32_to_cpu(entry->size);
> - return region->virt_base + le32_to_cpu(entry->offset);
> + *size = e_size;
> +
> + return region->virt_base + entry_offset;
The only change I see here is that you read entry->size regardless of
size being requested or not, so I don't see any "sanity checking" here.
> }
> }
>
> @@ -531,8 +545,12 @@ static void *qcom_smem_get_private(struct qcom_smem *smem,
> {
> struct smem_private_entry *e, *end;
> struct smem_partition_header *phdr;
> + void *item_ptr, *p_end;
> + u32 padding_data;
> + u32 e_size;
>
> phdr = (struct smem_partition_header __force *)part->virt_base;
> + p_end = (void *)phdr + part->size;
>
> e = phdr_to_first_uncached_entry(phdr);
> end = phdr_to_last_uncached_entry(phdr);
> @@ -542,36 +560,65 @@ static void *qcom_smem_get_private(struct qcom_smem *smem,
> 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);
> + if (size != NULL) {
> + e_size = le32_to_cpu(e->size);
> + padding_data = le16_to_cpu(e->padding_data);
>
> - return uncached_entry_to_item(e);
> + if (WARN_ON(e_size > part->size || padding_data > e_size))
> + return ERR_PTR(-EINVAL);
> +
> + *size = e_size - padding_data;
> + }
> +
> + item_ptr = uncached_entry_to_item(e);
> + 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, part->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);
> + if (size != NULL) {
> + e_size = le32_to_cpu(e->size);
> + padding_data = le16_to_cpu(e->padding_data);
> +
> + if (WARN_ON(e_size > part->size || padding_data > e_size))
> + return ERR_PTR(-EINVAL);
> +
> + *size = e_size - padding_data;
> + }
> +
> + item_ptr = cached_entry_to_item(e);
> + if (WARN_ON(item_ptr < (void *)phdr))
> + return ERR_PTR(-EINVAL);
>
> - return cached_entry_to_item(e);
> + return item_ptr;
> }
>
> e = cached_entry_next(e, part->cacheline);
> }
>
> + if (WARN_ON((void *)e < (void *)phdr))
> + return ERR_PTR(-EINVAL);
> +
> return ERR_PTR(-ENOENT);
>
> invalid_canary:
> @@ -648,14 +695,23 @@ int qcom_smem_get_free_space(unsigned host)
> phdr = part->virt_base;
> ret = le32_to_cpu(phdr->offset_free_cached) -
> le32_to_cpu(phdr->offset_free_uncached);
> +
> + if (ret > le32_to_cpu(part->size))
> + return -EINVAL;
> } else if (__smem->global_partition.virt_base) {
> part = &__smem->global_partition;
> phdr = part->virt_base;
> ret = le32_to_cpu(phdr->offset_free_cached) -
> le32_to_cpu(phdr->offset_free_uncached);
> +
> + if (ret > le32_to_cpu(part->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;
> @@ -918,13 +974,12 @@ qcom_smem_enumerate_partitions(struct qcom_smem *smem, u16 local_host)
> static int qcom_smem_map_toc(struct qcom_smem *smem, struct smem_region *region)
I presume this function was introduced in patch 1?
> {
> u32 ptable_start;
> - int ret;
Below changes doesn't affect "ret", so it should probably have been
removed in the previous patch.
>
> /* map starting 4K for smem header */
> - region->virt_base = devm_ioremap_wc(dev, region->aux_base, SZ_4K);
> + region->virt_base = devm_ioremap_wc(smem->dev, region->aux_base, SZ_4K);
I don't see "dev" in the scope here, did this compile after patch 1?
> ptable_start = region->aux_base + region->size - SZ_4K;
> /* map last 4k for toc */
> - smem->ptable = devm_ioremap_wc(dev, ptable_start, SZ_4K);
> + smem->ptable = devm_ioremap_wc(smem->dev, ptable_start, SZ_4K);
Ditto.
Regards,
Bjorn
>
> if (!region->virt_base || !smem->ptable)
> return -ENOMEM;
> --
> 2.7.4
>
