| Message ID | 20240117094432.1049168-2-masahisa.kojima@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | fix and refactoring of efi_disk.c | expand |
Hi Kojima-san On Wed, 17 Jan 2024 at 11:46, Masahisa Kojima <masahisa.kojima@linaro.org> wrote: > > efi_delete_handle() calls efi_purge_handle(), then it finally > frees the EFI handle. > Both diskobj and handle variables in efi_disk_remove() have > the same pointer, we can not access diskobj->dp after calling > efi_delete_handle(). > > This commit saves the struct efi_device_path pointer before > calling efi_delete_handle(). This commit also fixes the > missing free for volume member in struct efi_disk_obj. > > This commit also removes the container_of() calls, and > adds the TODO comment of missing efi_close_protocol() call > for the parent EFI_BLOCK_IO_PROTOCOL. > > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org> > --- > lib/efi_loader/efi_disk.c | 23 +++++++++++++++++------ > 1 file changed, 17 insertions(+), 6 deletions(-) > > diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c > index 013842f077..ce46c1092a 100644 > --- a/lib/efi_loader/efi_disk.c > +++ b/lib/efi_loader/efi_disk.c > @@ -707,35 +707,46 @@ int efi_disk_remove(void *ctx, struct event *event) > struct udevice *dev = event->data.dm.dev; > efi_handle_t handle; > struct blk_desc *desc; > + struct efi_device_path *dp = NULL; > struct efi_disk_obj *diskobj = NULL; > + struct efi_simple_file_system_protocol *volume = NULL; > efi_status_t ret; > > if (dev_tag_get_ptr(dev, DM_TAG_EFI, (void **)&handle)) > return 0; > > + if (!handle) > + return 0; > + Is there a chance dev_tag_get_ptr() will return a NULL handle? > id = device_get_uclass_id(dev); > switch (id) { > case UCLASS_BLK: > desc = dev_get_uclass_plat(dev); > if (desc && desc->uclass_id != UCLASS_EFI_LOADER) > - diskobj = container_of(handle, struct efi_disk_obj, > - header); > + diskobj = (struct efi_disk_obj *)handle; > break; > case UCLASS_PARTITION: > - diskobj = container_of(handle, struct efi_disk_obj, header); > + diskobj = (struct efi_disk_obj *)handle; > + > + /* TODO: closing the parent EFI_BLOCK_IO_PROTOCOL is missing. */ > + > break; > default: > return 0; > } > > + if (diskobj) { > + dp = diskobj->dp; > + volume = diskobj->volume; > + } > + > ret = efi_delete_handle(handle); Hmm, in which context are the dp and volume pointers allocated? The handle is now freed, can we reliably access dp and volume ptrs? > /* Do not delete DM device if there are still EFI drivers attached. */ > if (ret != EFI_SUCCESS) > return -1; > > - if (diskobj) > - efi_free_pool(diskobj->dp); > - > + efi_free_pool(dp); > + free(volume); > dev_tag_del(dev, DM_TAG_EFI); > > return 0; > -- > 2.34.1 > Thanks /Ilias
On Wed, 17 Jan 2024 at 20:52, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > Hi Kojima-san > > On Wed, 17 Jan 2024 at 11:46, Masahisa Kojima > <masahisa.kojima@linaro.org> wrote: > > > > efi_delete_handle() calls efi_purge_handle(), then it finally > > frees the EFI handle. > > Both diskobj and handle variables in efi_disk_remove() have > > the same pointer, we can not access diskobj->dp after calling > > efi_delete_handle(). > > > > This commit saves the struct efi_device_path pointer before > > calling efi_delete_handle(). This commit also fixes the > > missing free for volume member in struct efi_disk_obj. > > > > This commit also removes the container_of() calls, and > > adds the TODO comment of missing efi_close_protocol() call > > for the parent EFI_BLOCK_IO_PROTOCOL. > > > > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org> > > --- > > lib/efi_loader/efi_disk.c | 23 +++++++++++++++++------ > > 1 file changed, 17 insertions(+), 6 deletions(-) > > > > diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c > > index 013842f077..ce46c1092a 100644 > > --- a/lib/efi_loader/efi_disk.c > > +++ b/lib/efi_loader/efi_disk.c > > @@ -707,35 +707,46 @@ int efi_disk_remove(void *ctx, struct event *event) > > struct udevice *dev = event->data.dm.dev; > > efi_handle_t handle; > > struct blk_desc *desc; > > + struct efi_device_path *dp = NULL; > > struct efi_disk_obj *diskobj = NULL; > > + struct efi_simple_file_system_protocol *volume = NULL; > > efi_status_t ret; > > > > if (dev_tag_get_ptr(dev, DM_TAG_EFI, (void **)&handle)) > > return 0; > > > > + if (!handle) > > + return 0; > > + > > Is there a chance dev_tag_get_ptr() will return a NULL handle? There is no chance unless we actually set a NULL. Checking the return value of dev_tag_get_ptr above is enough, I will remove this. > > > id = device_get_uclass_id(dev); > > switch (id) { > > case UCLASS_BLK: > > desc = dev_get_uclass_plat(dev); > > if (desc && desc->uclass_id != UCLASS_EFI_LOADER) > > - diskobj = container_of(handle, struct efi_disk_obj, > > - header); > > + diskobj = (struct efi_disk_obj *)handle; > > break; > > case UCLASS_PARTITION: > > - diskobj = container_of(handle, struct efi_disk_obj, header); > > + diskobj = (struct efi_disk_obj *)handle; > > + > > + /* TODO: closing the parent EFI_BLOCK_IO_PROTOCOL is missing. */ > > + > > break; > > default: > > return 0; > > } > > > > + if (diskobj) { > > + dp = diskobj->dp; > > + volume = diskobj->volume; > > + } > > + > > ret = efi_delete_handle(handle); > > Hmm, in which context are the dp and volume pointers allocated? The > handle is now freed, can we reliably access dp and volume ptrs? dp and volume are allocated in efi_disk_add_dev() when the block device is probed. efi_delete_handle() will remove all protocols from the handle, efi_uninstall_protocol() checks whether protocol_interface is actually installed. So dp and volume should be freed after efi_delete_handle(). Thanks, Masahisa Kojima > > > /* Do not delete DM device if there are still EFI drivers attached. */ > > if (ret != EFI_SUCCESS) > > return -1; > > > > - if (diskobj) > > - efi_free_pool(diskobj->dp); > > - > > + efi_free_pool(dp); > > + free(volume); > > dev_tag_del(dev, DM_TAG_EFI); > > > > return 0; > > -- > > 2.34.1 > > > > Thanks > /Ilias
On Thu, 18 Jan 2024 at 02:16, Masahisa Kojima <masahisa.kojima@linaro.org> wrote: > > On Wed, 17 Jan 2024 at 20:52, Ilias Apalodimas > <ilias.apalodimas@linaro.org> wrote: > > > > Hi Kojima-san > > > > On Wed, 17 Jan 2024 at 11:46, Masahisa Kojima > > <masahisa.kojima@linaro.org> wrote: > > > > > > efi_delete_handle() calls efi_purge_handle(), then it finally > > > frees the EFI handle. > > > Both diskobj and handle variables in efi_disk_remove() have > > > the same pointer, we can not access diskobj->dp after calling > > > efi_delete_handle(). > > > > > > This commit saves the struct efi_device_path pointer before > > > calling efi_delete_handle(). This commit also fixes the > > > missing free for volume member in struct efi_disk_obj. > > > > > > This commit also removes the container_of() calls, and > > > adds the TODO comment of missing efi_close_protocol() call > > > for the parent EFI_BLOCK_IO_PROTOCOL. > > > > > > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org> > > > --- > > > lib/efi_loader/efi_disk.c | 23 +++++++++++++++++------ > > > 1 file changed, 17 insertions(+), 6 deletions(-) > > > > > > diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c > > > index 013842f077..ce46c1092a 100644 > > > --- a/lib/efi_loader/efi_disk.c > > > +++ b/lib/efi_loader/efi_disk.c > > > @@ -707,35 +707,46 @@ int efi_disk_remove(void *ctx, struct event *event) > > > struct udevice *dev = event->data.dm.dev; > > > efi_handle_t handle; > > > struct blk_desc *desc; > > > + struct efi_device_path *dp = NULL; > > > struct efi_disk_obj *diskobj = NULL; > > > + struct efi_simple_file_system_protocol *volume = NULL; > > > efi_status_t ret; > > > > > > if (dev_tag_get_ptr(dev, DM_TAG_EFI, (void **)&handle)) > > > return 0; > > > > > > + if (!handle) > > > + return 0; > > > + > > > > Is there a chance dev_tag_get_ptr() will return a NULL handle? > > There is no chance unless we actually set a NULL. > Checking the return value of dev_tag_get_ptr above is enough, I will > remove this. > > > > > > id = device_get_uclass_id(dev); > > > switch (id) { > > > case UCLASS_BLK: > > > desc = dev_get_uclass_plat(dev); > > > if (desc && desc->uclass_id != UCLASS_EFI_LOADER) > > > - diskobj = container_of(handle, struct efi_disk_obj, > > > - header); > > > + diskobj = (struct efi_disk_obj *)handle; > > > break; > > > case UCLASS_PARTITION: > > > - diskobj = container_of(handle, struct efi_disk_obj, header); > > > + diskobj = (struct efi_disk_obj *)handle; > > > + > > > + /* TODO: closing the parent EFI_BLOCK_IO_PROTOCOL is missing. */ > > > + > > > break; > > > default: > > > return 0; > > > } > > > > > > + if (diskobj) { > > > + dp = diskobj->dp; > > > + volume = diskobj->volume; > > > + } > > > + > > > ret = efi_delete_handle(handle); > > > > Hmm, in which context are the dp and volume pointers allocated? The > > handle is now freed, can we reliably access dp and volume ptrs? > > dp and volume are allocated in efi_disk_add_dev() when the block > device is probed. > efi_delete_handle() will remove all protocols from the handle, > efi_uninstall_protocol() > checks whether protocol_interface is actually installed. So dp and > volume should be > freed after efi_delete_handle(). Ok that makes sense, With the check above removed Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > > Thanks, > Masahisa Kojima > > > > > > /* Do not delete DM device if there are still EFI drivers attached. */ > > > if (ret != EFI_SUCCESS) > > > return -1; > > > > > > - if (diskobj) > > > - efi_free_pool(diskobj->dp); > > > - > > > + efi_free_pool(dp); > > > + free(volume); > > > dev_tag_del(dev, DM_TAG_EFI); > > > > > > return 0; > > > -- > > > 2.34.1 > > > > > > > Thanks > > /Ilias
diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c index 013842f077..ce46c1092a 100644 --- a/lib/efi_loader/efi_disk.c +++ b/lib/efi_loader/efi_disk.c @@ -707,35 +707,46 @@ int efi_disk_remove(void *ctx, struct event *event) struct udevice *dev = event->data.dm.dev; efi_handle_t handle; struct blk_desc *desc; + struct efi_device_path *dp = NULL; struct efi_disk_obj *diskobj = NULL; + struct efi_simple_file_system_protocol *volume = NULL; efi_status_t ret; if (dev_tag_get_ptr(dev, DM_TAG_EFI, (void **)&handle)) return 0; + if (!handle) + return 0; + id = device_get_uclass_id(dev); switch (id) { case UCLASS_BLK: desc = dev_get_uclass_plat(dev); if (desc && desc->uclass_id != UCLASS_EFI_LOADER) - diskobj = container_of(handle, struct efi_disk_obj, - header); + diskobj = (struct efi_disk_obj *)handle; break; case UCLASS_PARTITION: - diskobj = container_of(handle, struct efi_disk_obj, header); + diskobj = (struct efi_disk_obj *)handle; + + /* TODO: closing the parent EFI_BLOCK_IO_PROTOCOL is missing. */ + break; default: return 0; } + if (diskobj) { + dp = diskobj->dp; + volume = diskobj->volume; + } + ret = efi_delete_handle(handle); /* Do not delete DM device if there are still EFI drivers attached. */ if (ret != EFI_SUCCESS) return -1; - if (diskobj) - efi_free_pool(diskobj->dp); - + efi_free_pool(dp); + free(volume); dev_tag_del(dev, DM_TAG_EFI); return 0;
efi_delete_handle() calls efi_purge_handle(), then it finally frees the EFI handle. Both diskobj and handle variables in efi_disk_remove() have the same pointer, we can not access diskobj->dp after calling efi_delete_handle(). This commit saves the struct efi_device_path pointer before calling efi_delete_handle(). This commit also fixes the missing free for volume member in struct efi_disk_obj. This commit also removes the container_of() calls, and adds the TODO comment of missing efi_close_protocol() call for the parent EFI_BLOCK_IO_PROTOCOL. Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org> --- lib/efi_loader/efi_disk.c | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-)