On Wed, Mar 28, 2018 at 11:45:24AM +1100, Alexey Kardashevskiy wrote:
> 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.
Oh, right. Yeah, sorry, I don't know where to find that. Or even if
it was ever actually written out.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
Attachment:
signature.asc
Description: PGP signature
