Re: [PATCH 02/12] libfdt: Make fdt_check_header() more thorough

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]



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


[Index of Archives]     [Device Tree]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux