On Mon, Feb 10, 2020 at 06:04:47PM -0700, Simon Glass wrote:
> Hi David,
>
> On Sun, 9 Feb 2020 at 23:31, David Gibson <david@xxxxxxxxxxxxxxxxxxxxx> wrote:
> >
> > On Wed, Feb 05, 2020 at 10:40:30PM -0700, Simon Glass wrote:
> > > Support ASSUME_VALID_DTB to disable some sanity checks
> > >
> > > If we assume that the DTB itself is valid then we can skip some checks and
> > > save code space. Add various conditions to handle this.
> > >
> > > Signed-off-by: Simon Glass <sjg@xxxxxxxxxxxx>
> > > ---
> > >
> > > Changes in v5:
> > > - Split out VALID_DTB checks into a separate patch
> > >
> > > Changes in v4: None
> > > Changes in v3: None
> > > Changes in v2: None
> > >
> > > libfdt/fdt.c | 50 ++++++++++++++++++++++------------------
> > > libfdt/fdt_ro.c | 7 ++++--
> > > libfdt/fdt_rw.c | 7 +++++-
> > > libfdt/fdt_sw.c | 30 ++++++++++++++++--------
> > > libfdt/libfdt_internal.h | 7 ++++--
> > > 5 files changed, 64 insertions(+), 37 deletions(-)
> > >
> > > diff --git a/libfdt/fdt.c b/libfdt/fdt.c
> > > index 3e37a4b..03f2b7d 100644
> > > --- a/libfdt/fdt.c
> > > +++ b/libfdt/fdt.c
> > > @@ -81,38 +81,44 @@ int fdt_check_header(const void *fdt)
> > >
> > > if (fdt_magic(fdt) != FDT_MAGIC)
> > > return -FDT_ERR_BADMAGIC;
> > > - hdrsize = fdt_header_size(fdt);
> > > 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;
> > > + hdrsize = fdt_header_size(fdt);
> > > + if (!can_assume(VALID_DTB)) {
> > >
> > > - 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;
> > > + if ((fdt_totalsize(fdt) < hdrsize)
> > > + || (fdt_totalsize(fdt) > INT_MAX))
> > > + return -FDT_ERR_TRUNCATED;
> > >
> > > - /* Bounds check structure block */
> > > - if (fdt_version(fdt) < 17) {
> > > + /* Bounds check memrsv block */
> > > if (!check_off_(hdrsize, fdt_totalsize(fdt),
> > > - fdt_off_dt_struct(fdt)))
> > > + fdt_off_mem_rsvmap(fdt)))
> > > return -FDT_ERR_TRUNCATED;
> > > - } else {
> > > + }
> > > +
> > > + if (!can_assume(VALID_DTB)) {
> > > + /* Bounds check structure block */
> > > + if (fdt_version(fdt) < 17) {
> > > + if (!check_off_(hdrsize, fdt_totalsize(fdt),
> > > + fdt_off_dt_struct(fdt)))
> > > + return -FDT_ERR_TRUNCATED;
> > > + } else {
> > > + if (!check_block_(hdrsize, fdt_totalsize(fdt),
> > > + fdt_off_dt_struct(fdt),
> > > + fdt_size_dt_struct(fdt)))
> > > + return -FDT_ERR_TRUNCATED;
> > > + }
> > > +
> > > + /* Bounds check strings block */
> > > if (!check_block_(hdrsize, fdt_totalsize(fdt),
> > > - fdt_off_dt_struct(fdt),
> > > - fdt_size_dt_struct(fdt)))
> > > + fdt_off_dt_strings(fdt),
> > > + fdt_size_dt_strings(fdt)))
> > > return -FDT_ERR_TRUNCATED;
> > > }
> > >
> > > - /* Bounds check strings block */
> > > - if (!check_block_(hdrsize, fdt_totalsize(fdt),
> > > - fdt_off_dt_strings(fdt), fdt_size_dt_strings(fdt)))
> > > - return -FDT_ERR_TRUNCATED;
> > > -
> > > return 0;
> > > }
> > >
> > > @@ -142,7 +148,7 @@ uint32_t fdt_next_tag(const void *fdt, int startoffset, int *nextoffset)
> > >
> > > *nextoffset = -FDT_ERR_TRUNCATED;
> > > tagp = fdt_offset_ptr(fdt, offset, FDT_TAGSIZE);
> > > - if (!tagp)
> > > + if (!can_assume(VALID_DTB) && !tagp)
> > > return FDT_END; /* premature end */
> > > tag = fdt32_to_cpu(*tagp);
> > > offset += FDT_TAGSIZE;
> > > @@ -154,13 +160,13 @@ uint32_t fdt_next_tag(const void *fdt, int startoffset, int *nextoffset)
> > > do {
> > > p = fdt_offset_ptr(fdt, offset++, 1);
> > > } while (p && (*p != '\0'));
> > > - if (!p)
> > > + if (!can_assume(VALID_DTB) && !p)
> > > return FDT_END; /* premature end */
> > > break;
> > >
> > > case FDT_PROP:
> > > lenp = fdt_offset_ptr(fdt, offset, sizeof(*lenp));
> > > - if (!lenp)
> > > + if (!can_assume(VALID_DTB) && !lenp)
> > > return FDT_END; /* premature end */
> > > /* skip-name offset, length and value */
> > > offset += sizeof(struct fdt_property) - FDT_TAGSIZE
> > > diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
> > > index a5c2797..b41083f 100644
> > > --- a/libfdt/fdt_ro.c
> > > +++ b/libfdt/fdt_ro.c
> > > @@ -289,9 +289,12 @@ const char *fdt_get_name(const void *fdt, int nodeoffset, int *len)
> > > const char *nameptr;
> > > int err;
> > >
> > > - if (((err = fdt_ro_probe_(fdt)) < 0)
> > > - || ((err = fdt_check_node_offset_(fdt, nodeoffset)) < 0))
> > > + if (!can_assume(VALID_DTB)) {
> > > + if ((err = fdt_ro_probe_(fdt)) < 0)
> >
> > Seems like it might be cleaner to include the can_assume() check into
> > fdt_ro_probe_().
>
> OK.
>
> >
> > > goto fail;
> > > + if ((err = fdt_check_node_offset_(fdt, nodeoffset)) < 0)
> > > + goto fail;
> > > + }
> > >
> > > nameptr = nh->name;
> > >
> > > diff --git a/libfdt/fdt_rw.c b/libfdt/fdt_rw.c
> > > index 8795947..7a41a31 100644
> > > --- a/libfdt/fdt_rw.c
> > > +++ b/libfdt/fdt_rw.c
> > > @@ -13,6 +13,8 @@
> > > static int fdt_blocks_misordered_(const void *fdt,
> > > int mem_rsv_size, int struct_size)
> > > {
> > > + if (can_assume(VALID_DTB))
> > > + return false;
> >
> > Urgh... the spec doesn't actually specify a block order, so I'm not
> > sure we can put this one under VALID_DTB.
>
> But if this returns true then we return -FDT_ERR_BADLAYOUT in
> fdt_rw_probe_() below. So it does seem to be wrong.
Ah, right. So, that's true for all cases, except the call in
fdt_open_into(). The idea is that you have to call fdt_open_into()
before any of the other rw functions. fdt_open_into() will re-order
the blocks, if necessary, subsequent functions then require them in
the "standard" order.
So for the cases other than open_into() you can probably put this
under VALID_INPUT, I think it's reasonable to include "you've called
prerequisite functions correctly" under requiring suitable input.
You almost certainly also want to elide the block reordering code in
fdt_open_into(), though. That one probably wants another flag
> > > return (fdt_off_mem_rsvmap(fdt) < FDT_ALIGN(sizeof(struct fdt_header), 8))
> > > || (fdt_off_dt_struct(fdt) <
> > > (fdt_off_mem_rsvmap(fdt) + mem_rsv_size))
> > > @@ -24,6 +26,8 @@ static int fdt_blocks_misordered_(const void *fdt,
> > >
> > > static int fdt_rw_probe_(void *fdt)
> > > {
> > > + if (can_assume(VALID_DTB))
> > > + return 0;
> > > FDT_RO_PROBE(fdt);
> > >
> > > if (fdt_version(fdt) < 17)
> > > @@ -40,7 +44,8 @@ static int fdt_rw_probe_(void *fdt)
> > > #define FDT_RW_PROBE(fdt) \
> > > { \
> > > int err_; \
> > > - if ((err_ = fdt_rw_probe_(fdt)) != 0) \
> > > + if (!can_assume(VALID_DTB) && \
> > > + (err_ = fdt_rw_probe_(fdt)) != 0) \
> >
> >
> > You shouldn't need the test both inside fdt_rw_probe_() and in the
> > FDT_RW_PROBE() macro, no?
>
> OK. It saves a small amount of code space but we can look at it once
> we get something applied...
Ok, do you know why that is?
>
> >
> > > return err_; \
> > > }
> > >
>
> I'll hold off sending a new version until I hear back.
>
> Regards,
> Simon
>
--
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
