On 26/3/18 6:48 pm, David Gibson wrote:
> On Mon, Mar 26, 2018 at 04:54:11PM +1100, Alexey Kardashevskiy wrote:
>> On 26/3/18 10:25 am, David Gibson wrote:
>>> Currently fdt_check_header() performs only some rudimentary checks, which
>>> is not really what the name suggests. This strengthens fdt_check_header()
>>> to check as much about the blob as is possible from the header alone: as
>>> well as checking the magic number and version, it checks that the total
>>> size is sane, and that all the sub-blocks within the blob lie within the
>>> total size.
>>>
>>> * This broadens the meaning of FDT_ERR_TRUNCATED to cover all sorts of
>>> improperly terminated blocks as well as just a structure block without
>>> FDT_END.
>>>
>>> * This makes fdt_check_header() only succeed on "complete" blobs, not
>>> in-progress sequential write blobs. The only reason this didn't fail
>>> before was that this function used to be called by many RO functions
>>> which are supposed to also work on incomplete SW blobs.
>>>
>>> Signed-off-by: David Gibson <david@xxxxxxxxxxxxxxxxxxxxx>
>>> ---
>>> libfdt/fdt.c | 57 ++++++++++++++++++++++-
>>> libfdt/libfdt.h | 5 +-
>>> libfdt/libfdt_env.h | 1 +
>>> tests/.gitignore | 1 +
>>> tests/Makefile.tests | 2 +-
>>> tests/check_header.c | 128 +++++++++++++++++++++++++++++++++++++++++++++++++++
>>> tests/run_tests.sh | 3 ++
>>> 7 files changed, 193 insertions(+), 4 deletions(-)
>>> create mode 100644 tests/check_header.c
>>>
>>> diff --git a/libfdt/fdt.c b/libfdt/fdt.c
>>> index af2f513..4503e9c 100644
>>> --- a/libfdt/fdt.c
>>> +++ b/libfdt/fdt.c
>>> @@ -74,9 +74,64 @@ int fdt_ro_probe_(const void *fdt)
>>> return 0;
>>> }
>>>
>>> +static int check_off_(uint32_t hdrsize, uint32_t totalsize, uint32_t off)
>>
>> Make it return "bool"? When it is "int", 0 is rather "success" (like
>> fdt_check_header does). Same about check_block_ below.
>
> Yeah, it's logically a bool.
Ok. I just grepped "bool" and it showed up in dtc.h, this is why I brought
this up.
>
> So far I haven't used the bool type within libfdt, though, for fear of
> incompatibility on weird embedded compilers. It might be worth
> changing that, but that would be a standalone patch, rather than
> folded in with an unrelated change.
>
>>> +{
>>> + return (off >= hdrsize) && (off <= totalsize);
>>> +}
>>> +
>>> +static int check_block_(uint32_t hdrsize, uint32_t totalsize,
>>> + uint32_t base, uint32_t size)
>>> +{
>>> + if (!check_off_(hdrsize, totalsize, base))
>>> + return 0; /* block start out of bounds */
>>> + if ((base + size) < base)
>>> + return 0; /* overflow */
>>> + if (!check_off_(hdrsize, totalsize, base + size))
>>> + return 0; /* block end out of bounds */
>>> + return 1;
>>> +}
>>> +
>>> int fdt_check_header(const void *fdt)
>>> {
>>> - return fdt_ro_probe_(fdt);
>>> + size_t hdrsize = FDT_V16_SIZE;
>>> +
>>> + if (fdt_magic(fdt) != FDT_MAGIC)
>>> + return -FDT_ERR_BADMAGIC;
>>> + if ((fdt_version(fdt) < FDT_FIRST_SUPPORTED_VERSION)
>>> + || (fdt_last_comp_version(fdt) > FDT_LAST_SUPPORTED_VERSION))
>>> + return -FDT_ERR_BADVERSION;
>>> + if (fdt_version(fdt) < fdt_last_comp_version(fdt))
>>> + return -FDT_ERR_BADVERSION;
>>> +
>>> + if (fdt_version(fdt) >= 17)
>>> + hdrsize = FDT_V17_SIZE;
>>> +
>>> + if ((fdt_totalsize(fdt) < hdrsize)
>>> + || (fdt_totalsize(fdt) > INT_MAX))
>>> + return -FDT_ERR_TRUNCATED;
>>> +
>>> + /* Bounds check memrsv block */
>>> + if (!check_off_(hdrsize, fdt_totalsize(fdt), fdt_off_mem_rsvmap(fdt)))
>>> + return -FDT_ERR_TRUNCATED;
>>> +
>>> + /* Bounds check structure block */
>>> + if (fdt_version(fdt) < 17) {
>>
>>
>> 17, not 16? I do not have v16 handy though (do you? I'd have a copy), only
>> Devicetree Specification, Release 0.1, which seems to use the same
>> structure as v16.
>
> Yes. size_dt_struct was only added in v17 (in fact, I think that's
> the only change from v16 to v17).
Sooo you do not have v16, do you? :)
--
Alexey
Attachment:
signature.asc
Description: OpenPGP digital signature
