Re: [Minios-devel] [UNIKRAFT RFC PATCH] plat/common:Add helper to simplify accessing fdt for arm

[Julien Grall <julien.grall@xxxxxxx>]

Hi,

On 11/15/18 10:11 AM, Jianyong Wu (Arm Technology China) wrote:
> > -----Original Message-----
> > From: Julien Grall <julien.grall@xxxxxxx>
> > Sent: Thursday, November 15, 2018 5:52 PM
> > To: Jianyong Wu (Arm Technology China) <Jianyong.Wu@xxxxxxx>; minios-
> > devel@xxxxxxxxxxxxxxxxxxxx; simon.kuenzer@xxxxxxxxx
> > Cc: Kaly Xin (Arm Technology China) <Kaly.Xin@xxxxxxx>; nd
> > <nd@xxxxxxx>; Wei Chen (Arm Technology China) <Wei.Chen@xxxxxxx>
> > Subject: Re: [Minios-devel] [UNIKRAFT RFC PATCH] plat/common:Add helper
> > to simplify accessing fdt for arm
> >
> >
> >
> > On 11/15/18 6:09 AM, Jianyong Wu (Arm Technology China) wrote:
> > > Hi,
> > Hi,
> >
> >
> > > > > > +
> > > > > > +       UK_ASSERT(device != -1);
> > > > > > +        naddr = fdt_address_cells(dtb, device);
> > > > > > +        UK_ASSERT(naddr < FDT_MAX_NCELLS);
> > > > > > +
> > > > > > +        *nsize = fdt_size_cells(dtb, device);
> > > > > > +        UK_ASSERT(*nsize < FDT_MAX_NCELLS);
> > > > > > +
> > > > > > +        *regs = fdt_getprop(dtb, device, "reg", &prop_len);
> > > > > > +        prop_min_len = (int)sizeof(fdt32_t) * (naddr + *nsize);
> > > > > > +        UK_ASSERT(*regs != NULL && prop_len >= prop_min_len);
> > > > > This assert is not very useful for "regs" property describing more
> > > > > than
> > > > > 1 regions. I think it would make sense to move the check in the
> > > > > uk_dtb_read_term to check if the region requested by the caller is
> > correct.
> > > > Ok, I will check reg in uk_dtb_read_term.
> > > But, how to check reg in uk_dtb_read_term? I have an idea that check
> > > *(reg + index*(naddr+nsize) *4) Does that make sence?
> > You want to make sure you are not going to read past the size of the
> > property. So ((index + 1) * (naddr + nsize) * 4) < size should be fine.
> The "size" in this function is not the size of all reg, it is just the length 
> of one term like distributor in gic.
> We not get the all regs cell size in this function.
Oh sorry, I misread the code sorry.

We definitely want some safety here, so it is probably a call to rework 
the interface.
I remembered you dismissed in a early revision, but I think it is 
worthwhile to reconsider the following interface:
uk_dtb_read_reg(int node, unsigned int index, &size);

You can then add all the safety and also avoid to have to add more 
parameters to this function.
What do you think?

Cheers,

--
Julien Grall

_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel