Re: [PATCH 1/3] dm: core: Check ofnode_to_offset() return value

[Tom Rini <trini@konsulko.com>]

[-- Attachment #1: Type: text/plain, Size: 2340 bytes --]

On Fri, Nov 14, 2025 at 06:17:27AM +0100, Marek Vasut wrote:
> On 11/13/25 11:46 PM, Simon Glass wrote:
> 
> Hello Simon,
> 
> > > > > diff --git a/drivers/core/ofnode.c b/drivers/core/ofnode.c
> > > > > index 071d998a0a5..55c2b3a10bb 100644
> > > > > --- a/drivers/core/ofnode.c
> > > > > +++ b/drivers/core/ofnode.c
> > > > > @@ -335,7 +335,7 @@ bool ofnode_name_eq_unit(ofnode node, const char *name)
> > > > >    int ofnode_read_u8(ofnode node, const char *propname, u8 *outp)
> > > > >    {
> > > > >           const u8 *cell;
> > > > > -       int len;
> > > > > +       int len, off;
> > > > > 
> > > > >           assert(ofnode_valid(node));
> > > > >           log_debug("%s: %s: ", __func__, propname);
> > > > > @@ -343,8 +343,13 @@ int ofnode_read_u8(ofnode node, const char *propname, u8 *outp)
> > > > >           if (ofnode_is_np(node))
> > > > >                   return of_read_u8(ofnode_to_np(node), propname, outp);
> > > > > 
> > > > > -       cell = fdt_getprop(gd->fdt_blob, ofnode_to_offset(node), propname,
> > > > > -                          &len);
> > > > > +       off = ofnode_to_offset(node);
> > > > > +       if (off < 0) {
> > > > > +               log_debug("(not valid)\n");
> > > > > +               return -EINVAL;
> > > > > +       }
> > > > 
> > > > Ugh this is really horrible, sorry :-)
> > > > 
> > > > Better to put a wrapper around it than add all these checks, extra
> > > > code size, etc. Also in many cases the ofnode is known to be valid(TM)
> > > > so it might just be a waste.
> > > 
> > > How would you propose the wrapper should look like ?
> > > 
> > > Keep in mind, not everything is fdt_getprop( ... ofnode_to_offset() ...)
> > > in this patch.
> > 
> > I was thinking of
> > 
> > my_fdt_getprop(fdt, node, propname, lenp)
> > {
> > if (node is not valid)
> >     return NULL;
> > return fdt_getprop();
> > }
> 
> We would need a wrapper for almost every libfdt call, which isn't really
> better.
> 
> > What does fdt_getprop() return when the offset is -1 ?
> If can_assume(VALID_DTB) , then crash on NULL pointer dereference .
> If not, I think FDT_END (premature end) or an error code.

How are we now getting in this state is a question. Are we being passed
an unaligned DT and trying to work around/with it?

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]