On 7 October 2015 at 17:02, Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx> wrote:
> On 7 October 2015 at 16:49, Mark Salter <msalter@xxxxxxxxxx> wrote:
>> On Wed, 2015-10-07 at 09:35 +0100, Ard Biesheuvel wrote:
>>> This fixes two issues with the EFI libstub code that removes the memory
>>> nodes from the FDT:
>>> a) fdt_del_node() invalidates the iterator, so we should restart from the
>>> top after calling it;
>>> b) use fixed length of 6 when matching against 'memory' using strncmp(),
>>> since otherwise, substrings like 'm' or 'mem' could match as well.
>>>
>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx>
>>> ---
>>> drivers/firmware/efi/libstub/fdt.c | 5 +++--
>>> 1 file changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/firmware/efi/libstub/fdt.c b/drivers/firmware/efi/libstub/fdt.c
>>> index 0ef16923b4f0..2c7d09936479 100644
>>> --- a/drivers/firmware/efi/libstub/fdt.c
>>> +++ b/drivers/firmware/efi/libstub/fdt.c
>>> @@ -200,6 +200,7 @@ efi_status_t update_fdt(efi_system_table_t *sys_table, void *orig_fdt,
>>> * Delete any memory nodes present. We must delete nodes which
>>> * early_init_dt_scan_memory may try to use.
>>> */
>>> +restart:
>>> prev = 0;
>>> for (;;) {
>>> const char *type;
>>> @@ -210,9 +211,9 @@ efi_status_t update_fdt(efi_system_table_t *sys_table, void *orig_fdt,
>>> break;
>>>
>>> type = fdt_getprop(fdt, node, "device_type", &len);
>>> - if (type && strncmp(type, "memory", len) == 0) {
>>> + if (type && strncmp(type, "memory", 6) == 0) {
>>
>> I see what you're trying to fix here, but I think using 6 is wrong
>> also. A plain strcmp() would work as long as property is a string.
>>
>
> That is what Leif suggested as well, but it pulls in yet another
> string function into the stub, so I tried to avoid it. I think this
> particular case is safe, since the property names are in the string
> table, which is after the nodes in the DTB structure, so 'type' is
> guaranteed not to point right before the end of a mapped region.
>
... or is your concern about matching on longer strings that only
start with 'memory'?
I guess it makes sense to use strcmp() after all, only arm64 does not
have an asm implementation, so under the stricter rules that I am
proposing for fencing off the EFI stub code from the kernel proper, I
will have to go and add a strcmp() implementation to
drivers/firmware/efi/libstub/string.c.
But still better than this, I guess ...
>>
>>> fdt_del_node(fdt, node);
>>> - continue;
>>> + goto restart;
>>> }
>>>
>>> prev = node;
>>
>> Maybe do:
>>
>> if (type && strncmp(type, "memory", len) == 0) {
>> fdt_del_node(fdt, node);
>> - continue;
>> - }
>> -
>> - prev = node;
>> + prev = NULL;
>> + } else
>> + prev = node;
>> }
>>
>>
>> instead of adding a goto.
>>
>
> Yes, that is actually better. Thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
