[PATCH 1/1] efi_loader: efi_dp_part_node check dp_alloc return value

[Heinrich Schuchardt]

On 10/6/22 14:49, Ilias Apalodimas wrote:
> On Thu, 6 Oct 2022 at 15:18, Heinrich Schuchardt
> <heinrich.schuchardt at canonical.com> wrote:
>>
>> On 10/6/22 13:59, Ilias Apalodimas wrote:
>>> On Thu, 6 Oct 2022 at 14:45, Heinrich Schuchardt
>>> <heinrich.schuchardt at canonical.com> wrote:
>>>>
>>>> dp_alloc() may return NULL. This needs to be caught.
>>>>
>>>> Fixes: 98d48bdf415e ("efi_loader: provide a function to create a partition node")
>>>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt at canonical.com>
>>>> ---
>>>>    lib/efi_loader/efi_device_path.c | 3 ++-
>>>>    lib/efi_loader/efi_disk.c        | 5 +++++
>>>>    2 files changed, 7 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c
>>>> index ebffb77122..acae007f26 100644
>>>> --- a/lib/efi_loader/efi_device_path.c
>>>> +++ b/lib/efi_loader/efi_device_path.c
>>>> @@ -936,7 +936,8 @@ struct efi_device_path *efi_dp_part_node(struct blk_desc *desc, int part)
>>>>                   dpsize = sizeof(struct efi_device_path_hard_drive_path);
>>>>           buf = dp_alloc(dpsize);
>>>>
>>>> -       dp_part_node(buf, desc, part);
>>>> +       if (buf)
>>>> +               dp_part_node(buf, desc, part);
>>>>
>>>>           return buf;
>>>>    }
>>>> diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
>>>> index 39ea1a68a6..79b28097b6 100644
>>>> --- a/lib/efi_loader/efi_disk.c
>>>> +++ b/lib/efi_loader/efi_disk.c
>>>> @@ -415,6 +415,11 @@ static efi_status_t efi_disk_add_dev(
>>>>                   struct efi_handler *handler;
>>>>                   void *protocol_interface;
>>>>
>>>> +               if (!node) {
>>>> +                       ret = EFI_OUT_OF_RESOURCES;
>>>> +                       goto error;
>>>> +               }
>>>
>>> There's a diskobj that has been allocated here and a handle we added.
>>> We should clean those up too
>>
>> 512 error:
>> 513         efi_delete_handle(&diskobj->header);
>>
>> What is missing here?
> 
> If deleting the handle fails we won't free that memory

If deleting succeeds, we won't free it either. And we don't reclaim the 
memory on the success path.

Not reclaiming the device-path is a separate bug to what I am looking at 
here.

Best regards

Heinrich