| Message ID | 20210908213049.89268-1-ilias.apalodimas@linaro.org |
|---|---|
| State | Accepted |
| Commit | 0bf538ce0c6d7cdf68749425e6c9f7b729066367 |
| Headers | show |
| Series | efi_loader: Remove incorrect calls of EFI_CALL in TCG2 | expand |
On 9/8/21 11:30 PM, Ilias Apalodimas wrote: > There is two unneeded EFI_CALL references in tcg2_measure_pe_image(). > The first one in efi_search_protocol() and the second on in the device path > calculation. The second isn't even a function we should be calling, but a > pointer assignment, which happens to work with the existing macro. > > While at it switch the malloc call to a calloc, remove the unnecessary cast > and get rid of an unneeded if statement before copying the device path > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de> > --- > lib/efi_loader/efi_tcg2.c | 15 ++++++--------- > 1 file changed, 6 insertions(+), 9 deletions(-) > > diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c > index 1319a8b37868..d026af2b2350 100644 > --- a/lib/efi_loader/efi_tcg2.c > +++ b/lib/efi_loader/efi_tcg2.c > @@ -839,20 +839,19 @@ efi_status_t tcg2_measure_pe_image(void *efi, u64 efi_size, > if (ret != EFI_SUCCESS) > return ret; > > - ret = EFI_CALL(efi_search_protocol(&handle->header, > - &efi_guid_loaded_image_device_path, > - &handler)); > + ret = efi_search_protocol(&handle->header, > + &efi_guid_loaded_image_device_path, &handler); > if (ret != EFI_SUCCESS) > return ret; > > - device_path = EFI_CALL(handler->protocol_interface); > + device_path = handler->protocol_interface; > device_path_length = efi_dp_size(device_path); > if (device_path_length > 0) { > /* add end node size */ > device_path_length += sizeof(struct efi_device_path); > } > event_size = sizeof(struct uefi_image_load_event) + device_path_length; > - image_load_event = (struct uefi_image_load_event *)malloc(event_size); > + image_load_event = calloc(1, event_size); > if (!image_load_event) > return EFI_OUT_OF_RESOURCES; > > @@ -875,10 +874,8 @@ efi_status_t tcg2_measure_pe_image(void *efi, u64 efi_size, > goto out; > } > > - if (device_path_length > 0) { > - memcpy(image_load_event->device_path, device_path, > - device_path_length); > - } > + /* device_path_length might be zero */ > + memcpy(image_load_event->device_path, device_path, device_path_length); > > ret = tcg2_agile_log_append(pcr_index, event_type, &digest_list, > event_size, (u8 *)image_load_event); >
Heinrich, On Thu, Sep 09, 2021 at 12:30:49AM +0300, Ilias Apalodimas wrote: > There is two unneeded EFI_CALL references in tcg2_measure_pe_image(). > The first one in efi_search_protocol() and the second on in the device path > calculation. The second isn't even a function we should be calling, but a > pointer assignment, which happens to work with the existing macro. It is a quite common mistake. For most people, it is not very trivial when we should use EFI_CALL and when should not. Do you think that we should leave a note somewhere? It would be much better if we can check any occurrence of mismatch, say using EFI_CALL for a non-EFIAPI function, at compile time. -Takahiro Akashi > While at it switch the malloc call to a calloc, remove the unnecessary cast > and get rid of an unneeded if statement before copying the device path > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > --- > lib/efi_loader/efi_tcg2.c | 15 ++++++--------- > 1 file changed, 6 insertions(+), 9 deletions(-) > > diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c > index 1319a8b37868..d026af2b2350 100644 > --- a/lib/efi_loader/efi_tcg2.c > +++ b/lib/efi_loader/efi_tcg2.c > @@ -839,20 +839,19 @@ efi_status_t tcg2_measure_pe_image(void *efi, u64 efi_size, > if (ret != EFI_SUCCESS) > return ret; > > - ret = EFI_CALL(efi_search_protocol(&handle->header, > - &efi_guid_loaded_image_device_path, > - &handler)); > + ret = efi_search_protocol(&handle->header, > + &efi_guid_loaded_image_device_path, &handler); > if (ret != EFI_SUCCESS) > return ret; > > - device_path = EFI_CALL(handler->protocol_interface); > + device_path = handler->protocol_interface; > device_path_length = efi_dp_size(device_path); > if (device_path_length > 0) { > /* add end node size */ > device_path_length += sizeof(struct efi_device_path); > } > event_size = sizeof(struct uefi_image_load_event) + device_path_length; > - image_load_event = (struct uefi_image_load_event *)malloc(event_size); > + image_load_event = calloc(1, event_size); > if (!image_load_event) > return EFI_OUT_OF_RESOURCES; > > @@ -875,10 +874,8 @@ efi_status_t tcg2_measure_pe_image(void *efi, u64 efi_size, > goto out; > } > > - if (device_path_length > 0) { > - memcpy(image_load_event->device_path, device_path, > - device_path_length); > - } > + /* device_path_length might be zero */ > + memcpy(image_load_event->device_path, device_path, device_path_length); > > ret = tcg2_agile_log_append(pcr_index, event_type, &digest_list, > event_size, (u8 *)image_load_event); > -- > 2.33.0 >
diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c index 1319a8b37868..d026af2b2350 100644 --- a/lib/efi_loader/efi_tcg2.c +++ b/lib/efi_loader/efi_tcg2.c @@ -839,20 +839,19 @@ efi_status_t tcg2_measure_pe_image(void *efi, u64 efi_size, if (ret != EFI_SUCCESS) return ret; - ret = EFI_CALL(efi_search_protocol(&handle->header, - &efi_guid_loaded_image_device_path, - &handler)); + ret = efi_search_protocol(&handle->header, + &efi_guid_loaded_image_device_path, &handler); if (ret != EFI_SUCCESS) return ret; - device_path = EFI_CALL(handler->protocol_interface); + device_path = handler->protocol_interface; device_path_length = efi_dp_size(device_path); if (device_path_length > 0) { /* add end node size */ device_path_length += sizeof(struct efi_device_path); } event_size = sizeof(struct uefi_image_load_event) + device_path_length; - image_load_event = (struct uefi_image_load_event *)malloc(event_size); + image_load_event = calloc(1, event_size); if (!image_load_event) return EFI_OUT_OF_RESOURCES; @@ -875,10 +874,8 @@ efi_status_t tcg2_measure_pe_image(void *efi, u64 efi_size, goto out; } - if (device_path_length > 0) { - memcpy(image_load_event->device_path, device_path, - device_path_length); - } + /* device_path_length might be zero */ + memcpy(image_load_event->device_path, device_path, device_path_length); ret = tcg2_agile_log_append(pcr_index, event_type, &digest_list, event_size, (u8 *)image_load_event);
There is two unneeded EFI_CALL references in tcg2_measure_pe_image(). The first one in efi_search_protocol() and the second on in the device path calculation. The second isn't even a function we should be calling, but a pointer assignment, which happens to work with the existing macro. While at it switch the malloc call to a calloc, remove the unnecessary cast and get rid of an unneeded if statement before copying the device path Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> --- lib/efi_loader/efi_tcg2.c | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) -- 2.33.0