On 28/3/18 10:16 am, David Gibson wrote:
> On Tue, Mar 27, 2018 at 07:40:23PM +1100, Alexey Kardashevskiy wrote:
>> 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.
>
> Yes, bool is used in dtc, but for libfdt I have greater paranoia about
> compiler and library limitations, since it's supposed to be embeddable
> in weird environments.
>
>>> 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? :)
>
> I really don't know what you're getting at. If we have v17 or later,
> we use size_dt_struct we check that the whole structure block is in
> bounds, otherwise we just check the start offset since that's all we
> have.
I just wanted your v16 spec, mostly for self-education, that's 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
