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.
--
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
