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

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



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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.