On 4/4/18 9:06 pm, David Gibson wrote:
> On Tue, Mar 27, 2018 at 06:09:12PM +1100, 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.
>
> Ah. So, technically spapr_h_cas_compose_response() is just doing the
> wrong thing here. You're supposed to construct a mem reserve segment
> between fdt_create() and the first fdt_begin_node(), even if that's
> just a single call to fdt_finish_reservemap().
Fair enough, I've posted a patch for QEMU.
> That said, it would be good for libfdt to be more robust against
> that. Either fdt_begin_node() should return FDT_ERR_BADSTATE in this
> situatipon, or it should automatically finish off the reserve map.
Returning an error from fdt_begin_node() should do it. Also probably
including fdt_finish() as it looks like it is possible to call fdt_create()
and fdt_finish() without creating nodes in between (have not tested this).
Automation is supposed to fix only one place in QEMU for now and it just
does not seem worth it.
--
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
