On 27/3/18 6:09 pm, Alexey Kardashevskiy wrote:
> On 26/3/18 10:25 am, David Gibson wrote:
>> fdt_num_mem_rsv() and fdt_get_mem_rsv() currently don't sanity check their
>> parameters, or the memory reserve section offset in the header. That means
>> that on a corrupted blob they could access outside of the range of memory
>> that they should.
>>
>> This improves their safety checking, meaning they shouldn't access outside
>> the blob's bounds, even if its contents are badly corrupted.
>>
>> Signed-off-by: David Gibson <david@xxxxxxxxxxxxxxxxxxxxx>
>> ---
>> libfdt/fdt_ro.c | 33 ++++++++++++++++++++-----
>> tests/.gitignore | 1 +
>> tests/Makefile.tests | 2 +-
>> tests/run_tests.sh | 1 +
>> tests/testdata.h | 1 +
>> tests/trees.S | 20 +++++++++++++++
>> tests/truncated_memrsv.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++
>> 7 files changed, 114 insertions(+), 7 deletions(-)
>> create mode 100644 tests/truncated_memrsv.c
>>
>> diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
>> index d4cec0e..c74b962 100644
>> --- a/libfdt/fdt_ro.c
>> +++ b/libfdt/fdt_ro.c
>> @@ -170,21 +170,42 @@ uint32_t fdt_get_max_phandle(const void *fdt)
>> return 0;
>> }
>>
>> +static const struct fdt_reserve_entry *fdt_mem_rsv(const void *fdt, int n)
>> +{
>> + int offset = n * sizeof(struct fdt_reserve_entry);
>> + int absoffset = fdt_off_mem_rsvmap(fdt) + offset;
>> +
>> + if (absoffset < fdt_off_mem_rsvmap(fdt))
>> + return NULL;
>> + if (absoffset > fdt_totalsize(fdt) - sizeof(struct fdt_reserve_entry))
>> + return NULL;
>> + return fdt_mem_rsv_(fdt, n);
>> +}
>> +
>> int fdt_get_mem_rsv(const void *fdt, int n, uint64_t *address, uint64_t *size)
>> {
>> + const struct fdt_reserve_entry *re;
>> +
>> FDT_RO_PROBE(fdt);
>> - *address = fdt64_to_cpu(fdt_mem_rsv_(fdt, n)->address);
>> - *size = fdt64_to_cpu(fdt_mem_rsv_(fdt, n)->size);
>> + re = fdt_mem_rsv(fdt, n);
>> + if (!re)
>> + return -FDT_ERR_BADOFFSET;
>> +
>> + *address = fdt64_to_cpu(re->address);
>> + *size = fdt64_to_cpu(re->size);
>> return 0;
>> }
>>
>> int fdt_num_mem_rsv(const void *fdt)
>> {
>> - int i = 0;
>> + int i;
>> + const struct fdt_reserve_entry *re;
>>
>> - while (fdt64_to_cpu(fdt_mem_rsv_(fdt, i)->size) != 0)
>> - i++;
>> - return i;
>> + for (i = 0; (re = fdt_mem_rsv(fdt, i)) != NULL; i++) {
>> + if (fdt64_to_cpu(re->size) == 0)
>> + return i;
>> + }
>> + return -FDT_ERR_TRUNCATED;
>
>
> QEMU's spapr_h_cas_compose_response() crashes on this one as it calls
> fdt_open_into() and that guy does not check if fdt_num_mem_rsv() fails and
> it does as there is no any reserved entry, even an empty one in the end of
> the list.
>
> fdt_create() could create an empty record or fdt_open_into() could test for
> an error, not sure which one is right here.
Handling these reserved areas is a bit dodgy, in general.
spapr_h_cas_compose_response():
/* Create skeleton */
fdt_skel = g_malloc0(size);
_FDT((fdt_create(fdt_skel, size)));
_FDT((fdt_begin_node(fdt_skel, "")));
_FDT((fdt_end_node(fdt_skel)));
_FDT((fdt_finish(fdt_skel)));
This produces:
(gdb) p *(struct fdt_header *)fdt_skel
$6 = {
magic = 0xedfe0dd0,
totalsize = 0x40000000,
off_dt_struct = 0x30000000,
off_dt_strings = 0x40000000,
off_mem_rsvmap = 0x30000000,
version = 0x11000000,
last_comp_version = 0x2000000,
boot_cpuid_phys = 0x0,
size_dt_strings = 0x0,
size_dt_struct = 0x10000000
off_mem_rsvmap == off_dt_struct. This just does not seem right as
size_dt_struct!=0 so there is an overlap.
--
Alexey
--
To unsubscribe from this list: send the line "unsubscribe devicetree-compiler" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
