| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <59d7f837-acb7-8ee0-ef47-f38f0c798d26@redhat.com>
Date: Tue, 15 Jan 2019 11:11:52 -0800
From: Laura Abbott <labbott@...hat.com>
To: "Andrew F. Davis" <afd@...com>,
Sumit Semwal <sumit.semwal@...aro.org>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Arve Hjønnevåg <arve@...roid.com>
Cc: devel@...verdev.osuosl.org, dri-devel@...ts.freedesktop.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH 14/14] staging: android: ion: Add UNMAPPED heap type and
helper
On 1/15/19 10:43 AM, Laura Abbott wrote:
> On 1/15/19 7:58 AM, Andrew F. Davis wrote:
>> On 1/14/19 8:32 PM, Laura Abbott wrote:
>>> On 1/11/19 10:05 AM, Andrew F. Davis wrote:
>>>> The "unmapped" heap is very similar to the carveout heap except
>>>> the backing memory is presumed to be unmappable by the host, in
>>>> my specific case due to firewalls. This memory can still be
>>>> allocated from and used by devices that do have access to the
>>>> backing memory.
>>>>
>>>> Based originally on the secure/unmapped heap from Linaro for
>>>> the OP-TEE SDP implementation, this was re-written to match
>>>> the carveout heap helper code.
>>>>
>>>> Suggested-by: Etienne Carriere <etienne.carriere@...aro.org>
>>>> Signed-off-by: Andrew F. Davis <afd@...com>
>>>> ---
>>>> drivers/staging/android/ion/Kconfig | 10 ++
>>>> drivers/staging/android/ion/Makefile | 1 +
>>>> drivers/staging/android/ion/ion.h | 16 +++
>>>> .../staging/android/ion/ion_unmapped_heap.c | 123 ++++++++++++++++++
>>>> drivers/staging/android/uapi/ion.h | 3 +
>>>> 5 files changed, 153 insertions(+)
>>>> create mode 100644 drivers/staging/android/ion/ion_unmapped_heap.c
>>>>
>>>> diff --git a/drivers/staging/android/ion/Kconfig
>>>> b/drivers/staging/android/ion/Kconfig
>>>> index 0fdda6f62953..a117b8b91b14 100644
>>>> --- a/drivers/staging/android/ion/Kconfig
>>>> +++ b/drivers/staging/android/ion/Kconfig
>>>> @@ -42,3 +42,13 @@ config ION_CMA_HEAP
>>>> Choose this option to enable CMA heaps with Ion. This heap is
>>>> backed
>>>> by the Contiguous Memory Allocator (CMA). If your system has
>>>> these
>>>> regions, you should say Y here.
>>>> +
>>>> +config ION_UNMAPPED_HEAP
>>>> + bool "ION unmapped heap support"
>>>> + depends on ION
>>>> + help
>>>> + Choose this option to enable UNMAPPED heaps with Ion. This heap is
>>>> + backed in specific memory pools, carveout from the Linux memory.
>>>> + Unlike carveout heaps these are assumed to be not mappable by
>>>> + kernel or user-space.
>>>> + Unless you know your system has these regions, you should say N
>>>> here.
>>>> diff --git a/drivers/staging/android/ion/Makefile
>>>> b/drivers/staging/android/ion/Makefile
>>>> index 17f3a7569e3d..c71a1f3de581 100644
>>>> --- a/drivers/staging/android/ion/Makefile
>>>> +++ b/drivers/staging/android/ion/Makefile
>>>> @@ -4,3 +4,4 @@ obj-$(CONFIG_ION_SYSTEM_HEAP) += ion_system_heap.o
>>>> ion_page_pool.o
>>>> obj-$(CONFIG_ION_CARVEOUT_HEAP) += ion_carveout_heap.o
>>>> obj-$(CONFIG_ION_CHUNK_HEAP) += ion_chunk_heap.o
>>>> obj-$(CONFIG_ION_CMA_HEAP) += ion_cma_heap.o
>>>> +obj-$(CONFIG_ION_UNMAPPED_HEAP) += ion_unmapped_heap.o
>>>> diff --git a/drivers/staging/android/ion/ion.h
>>>> b/drivers/staging/android/ion/ion.h
>>>> index 97b2876b165a..ce74332018ba 100644
>>>> --- a/drivers/staging/android/ion/ion.h
>>>> +++ b/drivers/staging/android/ion/ion.h
>>>> @@ -341,4 +341,20 @@ static inline struct ion_heap
>>>> *ion_chunk_heap_create(phys_addr_t base, size_t si
>>>> }
>>>> #endif
>>>> +#ifdef CONFIG_ION_UNMAPPED_HEAP
>>>> +/**
>>>> + * ion_unmapped_heap_create
>>>> + * @base: base address of carveout memory
>>>> + * @size: size of carveout memory region
>>>> + *
>>>> + * Creates an unmapped ion_heap using the passed in data
>>>> + */
>>>> +struct ion_heap *ion_unmapped_heap_create(phys_addr_t base, size_t
>>>> size);
>>>> +#else
>>>> +struct ion_heap *ion_unmapped_heap_create(phys_addr_t base, size_t size)
>>>> +{
>>>> + return ERR_PTR(-ENODEV);
>>>> +}
>>>> +#endif
>>>> +
>>>> #endif /* _ION_H */
>>>> diff --git a/drivers/staging/android/ion/ion_unmapped_heap.c
>>>> b/drivers/staging/android/ion/ion_unmapped_heap.c
>>>> new file mode 100644
>>>> index 000000000000..7602b659c2ec
>>>> --- /dev/null
>>>> +++ b/drivers/staging/android/ion/ion_unmapped_heap.c
>>>> @@ -0,0 +1,123 @@
>>>> +// SPDX-License-Identifier: GPL-2.0
>>>> +/*
>>>> + * ION Memory Allocator unmapped heap helper
>>>> + *
>>>> + * Copyright (C) 2015-2016 Texas Instruments Incorporated -
>>>> http://www.ti.com/
>>>> + * Andrew F. Davis <afd@...com>
>>>> + *
>>>> + * ION "unmapped" heaps are physical memory heaps not by default
>>>> mapped into
>>>> + * a virtual address space. The buffer owner can explicitly request
>>>> kernel
>>>> + * space mappings but the underlying memory may still not be
>>>> accessible for
>>>> + * various reasons, such as firewalls.
>>>> + */
>>>> +
>>>> +#include <linux/err.h>
>>>> +#include <linux/genalloc.h>
>>>> +#include <linux/scatterlist.h>
>>>> +#include <linux/slab.h>
>>>> +
>>>> +#include "ion.h"
>>>> +
>>>> +#define ION_UNMAPPED_ALLOCATE_FAIL -1
>>>> +
>>>> +struct ion_unmapped_heap {
>>>> + struct ion_heap heap;
>>>> + struct gen_pool *pool;
>>>> +};
>>>> +
>>>> +static phys_addr_t ion_unmapped_allocate(struct ion_heap *heap,
>>>> + unsigned long size)
>>>> +{
>>>> + struct ion_unmapped_heap *unmapped_heap =
>>>> + container_of(heap, struct ion_unmapped_heap, heap);
>>>> + unsigned long offset;
>>>> +
>>>> + offset = gen_pool_alloc(unmapped_heap->pool, size);
>>>> + if (!offset)
>>>> + return ION_UNMAPPED_ALLOCATE_FAIL;
>>>> +
>>>> + return offset;
>>>> +}
>>>> +
>>>> +static void ion_unmapped_free(struct ion_heap *heap, phys_addr_t addr,
>>>> + unsigned long size)
>>>> +{
>>>> + struct ion_unmapped_heap *unmapped_heap =
>>>> + container_of(heap, struct ion_unmapped_heap, heap);
>>>> +
>>>> + gen_pool_free(unmapped_heap->pool, addr, size);
>>>> +}
>>>> +
>>>> +static int ion_unmapped_heap_allocate(struct ion_heap *heap,
>>>> + struct ion_buffer *buffer,
>>>> + unsigned long size,
>>>> + unsigned long flags)
>>>> +{
>>>> + struct sg_table *table;
>>>> + phys_addr_t paddr;
>>>> + int ret;
>>>> +
>>>> + table = kmalloc(sizeof(*table), GFP_KERNEL);
>>>> + if (!table)
>>>> + return -ENOMEM;
>>>> + ret = sg_alloc_table(table, 1, GFP_KERNEL);
>>>> + if (ret)
>>>> + goto err_free;
>>>> +
>>>> + paddr = ion_unmapped_allocate(heap, size);
>>>> + if (paddr == ION_UNMAPPED_ALLOCATE_FAIL) {
>>>> + ret = -ENOMEM;
>>>> + goto err_free_table;
>>>> + }
>>>> +
>>>> + sg_set_page(table->sgl, pfn_to_page(PFN_DOWN(paddr)), size, 0);
>>>> + buffer->sg_table = table;
>>>> +
>>>
>>>
>>> If this memory is actually unmapped this is not going to work because
>>> the struct page will not be valid.
>>>
>>
>> If it will never get mapped then it doesn't need a valid struct page as
>> far as I can tell. We only use it as a marker to where the start of
>> backing memory is, and that is calculated based on the struct page
>> pointer address, not its contents.
>>
>
> You can't rely on pfn_to_page returning a valid page pointer if the
> pfn doesn't point to reserved memory. Even if you aren't relying
> on the contents, that's no guarantee you can use that to to get
> the valid pfn back. You can get away with that sometimes but it's
> not correct to rely on it all the time.
>
I found https://lore.kernel.org/lkml/20190111181731.11782-1-hch@lst.de/T/#u
which I think is closer to what we might want to be looking at.
>>>> + return 0;
>>>> +
>>>> +err_free_table:
>>>> + sg_free_table(table);
>>>> +err_free:
>>>> + kfree(table);
>>>> + return ret;
>>>> +}
>>>> +
>>>> +static void ion_unmapped_heap_free(struct ion_buffer *buffer)
>>>> +{
>>>> + struct ion_heap *heap = buffer->heap;
>>>> + struct sg_table *table = buffer->sg_table;
>>>> + struct page *page = sg_page(table->sgl);
>>>> + phys_addr_t paddr = PFN_PHYS(page_to_pfn(page));
>>>> +
>>>> + ion_unmapped_free(heap, paddr, buffer->size);
>>>> + sg_free_table(buffer->sg_table);
>>>> + kfree(buffer->sg_table);
>>>> +}
>>>> +
>>>> +static struct ion_heap_ops unmapped_heap_ops = {
>>>> + .allocate = ion_unmapped_heap_allocate,
>>>> + .free = ion_unmapped_heap_free,
>>>> + /* no .map_user, user mapping of unmapped heaps not allowed */
>>>> + .map_kernel = ion_heap_map_kernel,
>>>> + .unmap_kernel = ion_heap_unmap_kernel,
>>>> +};
>>>> +
>>>> +struct ion_heap *ion_unmapped_heap_create(phys_addr_t base, size_t size)
>>>> +{
>>>> + struct ion_unmapped_heap *unmapped_heap;
>>>> +
>>>> + unmapped_heap = kzalloc(sizeof(*unmapped_heap), GFP_KERNEL);
>>>> + if (!unmapped_heap)
>>>> + return ERR_PTR(-ENOMEM);
>>>> +
>>>> + unmapped_heap->pool = gen_pool_create(PAGE_SHIFT, -1);
>>>> + if (!unmapped_heap->pool) {
>>>> + kfree(unmapped_heap);
>>>> + return ERR_PTR(-ENOMEM);
>>>> + }
>>>> + gen_pool_add(unmapped_heap->pool, base, size, -1);
>>>> + unmapped_heap->heap.ops = &unmapped_heap_ops;
>>>> + unmapped_heap->heap.type = ION_HEAP_TYPE_UNMAPPED;
>>>> +
>>>> + return &unmapped_heap->heap;
>>>> +}
>>>> diff --git a/drivers/staging/android/uapi/ion.h
>>>> b/drivers/staging/android/uapi/ion.h
>>>> index 5d7009884c13..d5f98bc5f340 100644
>>>> --- a/drivers/staging/android/uapi/ion.h
>>>> +++ b/drivers/staging/android/uapi/ion.h
>>>> @@ -19,6 +19,8 @@
>>>> * carveout heap, allocations are physically
>>>> * contiguous
>>>> * @ION_HEAP_TYPE_DMA: memory allocated via DMA API
>>>> + * @ION_HEAP_TYPE_UNMAPPED: memory not intended to be mapped into
>>>> the
>>>> + * linux address space unless for debug cases
>>>> * @ION_NUM_HEAPS: helper for iterating over heaps, a bit mask
>>>> * is used to identify the heaps, so only 32
>>>> * total heap types are supported
>>>> @@ -29,6 +31,7 @@ enum ion_heap_type {
>>>> ION_HEAP_TYPE_CARVEOUT,
>>>> ION_HEAP_TYPE_CHUNK,
>>>> ION_HEAP_TYPE_DMA,
>>>> + ION_HEAP_TYPE_UNMAPPED,
>>>> ION_HEAP_TYPE_CUSTOM, /*
>>>> * must be last so device specific heaps always
>>>> * are at the end of this enum
>>>>
>>>
>>> Overall this seems way too similar to the carveout heap
>>> to justify adding another heap type. It also still missing
>>> the part of where exactly you call ion_unmapped_heap_create.
>>> Figuring that out is one of the blocking items for moving
>>> Ion out of staging.
>>>
>>
>> I agree with this being almost a 1:1 copy of the carveout heap, I'm just
>> not sure of a good way to do this without a new heap type. Adding flags
>> to the existing carveout type seem a bit messy. Plus then those flags
>> should be valid for other heap types, gets complicated quickly..
>>
>> I'll reply to the second part in your other top level response.
>>
>> Andrew
>>
>>> Thanks,
>>> Laura
>
Powered by blists - more mailing lists