| Message ID | 20230528230351.168210-1-luzmaximilian@gmail.com |
|---|---|
| Headers | show |
| Series | firmware: Add support for Qualcomm UEFI Secure Application | expand |
On Mon, May 29, 2023 at 01:03:48AM +0200, Maximilian Luz wrote: > Add a ucs2_strlcpy() function for UCS-2 strings. The behavior is > equivalent to the standard strlcpy() function, just for 16-bit character > UCS-2 strings. Eek, no. strlcpy() is dangerous in multiple ways[1]. Please implement strscpy() (i.e. use strnlen(), negative error on truncation, etc). Additionally, it'd be nice of the ucs2 helpers here also implemented the rest of the CONFIG_FORTIFY_SOURCE mitigations (i.e. checking for source and destination buffer size overflows at compile-time and run-time with __builtin_object_size() and __builtin_dynamoc_object_size() respectively). -Kees [1] https://docs.kernel.org/process/deprecated.html#strlcpy
On Tue, 30 May 2023 at 18:15, Maximilian Luz <luzmaximilian@gmail.com> wrote: > > On 5/30/23 17:25, Kees Cook wrote: > > On Mon, May 29, 2023 at 01:03:48AM +0200, Maximilian Luz wrote: > >> Add a ucs2_strlcpy() function for UCS-2 strings. The behavior is > >> equivalent to the standard strlcpy() function, just for 16-bit character > >> UCS-2 strings. > > > > Eek, no. strlcpy() is dangerous in multiple ways[1]. Please implement > > strscpy() (i.e. use strnlen(), negative error on truncation, etc). > > Right, make sense, thanks. Somehow I missed that the kernel has a better > function than the C stdlib for that... > > > Additionally, it'd be nice of the ucs2 helpers here also implemented the > > rest of the CONFIG_FORTIFY_SOURCE mitigations (i.e. checking for source > > and destination buffer size overflows at compile-time and run-time with > > __builtin_object_size() and __builtin_dynamoc_object_size() respectively). > > I can certainly try that, but I think this might be better suited for a > follow-up series, given that we then should also add those to the other > helpers. > Agreed. Let's log the followup work as a kspp work item, no need to make that part of this series. Thanks,
On Tue, May 30, 2023 at 06:17:35PM +0200, Ard Biesheuvel wrote: > On Tue, 30 May 2023 at 18:15, Maximilian Luz <luzmaximilian@gmail.com> wrote: > > > > On 5/30/23 17:25, Kees Cook wrote: > > > On Mon, May 29, 2023 at 01:03:48AM +0200, Maximilian Luz wrote: > > >> Add a ucs2_strlcpy() function for UCS-2 strings. The behavior is > > >> equivalent to the standard strlcpy() function, just for 16-bit character > > >> UCS-2 strings. > > > > > > Eek, no. strlcpy() is dangerous in multiple ways[1]. Please implement > > > strscpy() (i.e. use strnlen(), negative error on truncation, etc). > > > > Right, make sense, thanks. Somehow I missed that the kernel has a better > > function than the C stdlib for that... > > > > > Additionally, it'd be nice of the ucs2 helpers here also implemented the > > > rest of the CONFIG_FORTIFY_SOURCE mitigations (i.e. checking for source > > > and destination buffer size overflows at compile-time and run-time with > > > __builtin_object_size() and __builtin_dynamoc_object_size() respectively). > > > > I can certainly try that, but I think this might be better suited for a > > follow-up series, given that we then should also add those to the other > > helpers. > > > > Agreed. Let's log the followup work as a kspp work item, no need to > make that part of this series. Yeah, that's fine. Can you please open a KSSP issue for it so we don't forget? :)
On Mon, May 29, 2023 at 01:03:49AM +0200, Maximilian Luz wrote: > When setting-up the IRQ goes wrong, the __scm pointer currently remains > set even though we fail to probe the driver successfully. Due to this, > access to __scm may go wrong since associated resources (clocks, ...) > have been released. Therefore, clear the __scm pointer when setting-up > the IRQ fails. > > Fixes: 6bf325992236 ("firmware: qcom: scm: Add wait-queue handling logic") > Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com> > --- > > Patch introduced in v4 > > --- > drivers/firmware/qcom_scm.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c > index fde33acd46b7..d0070b833889 100644 > --- a/drivers/firmware/qcom_scm.c > +++ b/drivers/firmware/qcom_scm.c > @@ -1488,8 +1488,10 @@ static int qcom_scm_probe(struct platform_device *pdev) > } else { > ret = devm_request_threaded_irq(__scm->dev, irq, NULL, qcom_scm_irq_handler, > IRQF_ONESHOT, "qcom-scm", __scm); > - if (ret < 0) > + if (ret < 0) { > + __scm = NULL; This looks fragile at best. Clients use qcom_scm_is_available() to see if __scm is available and do not expect it to go away once it is live. It looks like you can hold off on initialising __scm until you've requested the interrupt, either by using IRQ_NOAUTOEN or fixing qcom_scm_waitq_wakeup() so that it doesn't use __scm directly. That would also take care of the previous branch which may also leave __scm set after the structure itself has been released on errors. You'll have similar problems when registering qseecom which currently depend on __scm being set, though. Clearing the pointer in that case is clearly broken as you currently rely on devres for deregistering the aux clients on errors (i.e. the clients using __scm are still registered when you clear the pointer in patch 3/4). > return dev_err_probe(scm->dev, ret, "Failed to request qcom-scm irq\n"); > + } > } > > __get_convention(); Johan
On Mon, May 29, 2023 at 01:03:51AM +0200, Maximilian Luz wrote: > On platforms using the Qualcomm UEFI Secure Application (uefisecapp), > EFI variables cannot be accessed via the standard interface in EFI > runtime mode. The respective functions return EFI_UNSUPPORTED. On these > platforms, we instead need to talk to uefisecapp. This commit provides > support for this and registers the respective efivars operations to > access EFI variables from the kernel. > > Communication with uefisecapp follows the Qualcomm QSEECOM / Secure OS > conventions via the respective SCM call interface. This is also the > reason why variable access works normally while boot services are > active. During this time, said SCM interface is managed by the boot > services. When calling ExitBootServices(), the ownership is transferred > to the kernel. Therefore, UEFI must not use that interface itself (as > multiple parties accessing this interface at the same time may lead to > complications) and cannot access variables for us. > > Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com> > --- > > Changes in v4: > - Adapt to changes in DMA allocation in patch 3. > - Rework alignment: Use native alignment of types instead of a general > 8 byte alignment. While the windows driver uses 8 byte alignment for > GUIDs, the native (4 byte) alignment seems to work fine here. > - Add a helper macro for determining size and layout of request and > response buffers, taking care of proper alignment. > - Implement support for EFI's query_variable_info() call, which is now > supported by the kernel (and expected by efivarfs). > - Move UCS-2 string helpers to lib/ucs2_string.c > +config QCOM_QSEECOM_UEFISECAPP > + tristate "Qualcomm SEE UEFI Secure App client driver" > + select QCOM_SCM > + select QCOM_SCM_QSEECOM This should just be depends on QCOM_SCM_QSEECOM Using select should generally be avoided. > + depends on EFI > + help > + Various Qualcomm SoCs do not allow direct access to EFI variables. > + Instead, these need to be accessed via the UEFI Secure Application > + (uefisecapp), residing in the Secure Execution Environment (SEE). > + > + This module provides a client driver for uefisecapp, installing efivar > + operations to allow the kernel accessing EFI variables, and via that also > + provide user-space with access to EFI variables via efivarfs. > + > + Select M or Y here to provide access to EFI variables on the > + aforementioned platforms. We still have not really sorted how best to handle modular efivars implementations as userspace may try to mount efivarfs before the module has been loaded... Perhaps require it to be built-in for now? > + > config SYSFB > bool > select BOOT_VESA_SUPPORT > +/* -- Alighment helpers ----------------------------------------------------- */ typo: Alignment > + > +/* > + * Helper macro to ensure proper alignment of types (fields and arrays) when > + * stored in some (contiguous) buffer. > + * > + * Note: The driver from which this one has been reverse-engineered expects an > + * alignment of 8 bytes (64 bits) for GUIDs. Our definition of efi_guid_t, > + * however, has an alignment of 4 byte (32 bits). So far, this seems to work > + * fine here. See also the comment on the typedef of efi_guid_t. > + */ > +#define qcuefi_buf_align_fields(fields...) \ > + ({ \ > + size_t __len = 0; \ > + fields \ > + __len; \ > + }) > + > +#define __field_impl(size, align, offset) \ > + ({ \ > + size_t *__offset = (offset); \ > + size_t __aligned; \ > + \ > + __aligned = ALIGN(__len, align); \ > + __len = __aligned + (size); \ > + \ > + if (__offset) \ > + *__offset = __aligned; \ > + }); > + > +#define __array_offs(type, count, offset) \ > + __field_impl(sizeof(type) * (count), __alignof__(type), offset) > + > +#define __array(type, count) __array_offs(type, count, NULL) > +#define __field_offs(type, offset) __array_offs(type, 1, offset) > +#define __field(type) __array_offs(type, 1, NULL) Heh. This is some nice macro magic. :) > +static efi_status_t qsee_uefi_get_variable(struct qcuefi_client *qcuefi, const efi_char16_t *name, > + const efi_guid_t *guid, u32 *attributes, > + unsigned long *data_size, void *data) > +{ > + struct qsee_req_uefi_get_variable *req_data; > + struct qsee_rsp_uefi_get_variable *rsp_data; > + unsigned long buffer_size = *data_size; > + efi_status_t efi_status = EFI_SUCCESS; > + unsigned long name_length; > + size_t guid_offs; > + size_t name_offs; > + size_t req_size; > + size_t rsp_size; > + int status; > + > + /* Validation: We need a name and GUID. */ > + if (!name || !guid) > + return EFI_INVALID_PARAMETER; > + > + /* Get length of name and ensure that the name is not truncated. */ > + name_length = ucs2_strnlen(name, QSEE_MAX_NAME_LEN) + 1; > + if (name_length > QSEE_MAX_NAME_LEN) > + return EFI_INVALID_PARAMETER; > + > + /* Validation: We need a buffer if the buffer_size is nonzero. */ > + if (buffer_size && !data) > + return EFI_INVALID_PARAMETER; > + > + /* Compute required buffer size for request. */ > + req_size = qcuefi_buf_align_fields( > + __field(*req_data) > + __array_offs(*name, name_length, &name_offs) > + __field_offs(*guid, &guid_offs) > + ); > + > + /* Compute required buffer size for response. */ > + rsp_size = qcuefi_buf_align_fields( > + __field(*rsp_data) > + __array(u8, buffer_size) > + ); > + > + /* Allocate request buffer. */ > + req_data = kzalloc(req_size, GFP_KERNEL); > + if (!req_data) { > + efi_status = EFI_OUT_OF_RESOURCES; > + goto out; > + } > + > + /* Allocate response buffer. */ > + rsp_data = kzalloc(rsp_size, GFP_KERNEL); > + if (!rsp_data) { > + efi_status = EFI_OUT_OF_RESOURCES; > + goto out_free_req; > + } > + > + /* Set up request data. */ > + req_data->command_id = QSEE_CMD_UEFI_GET_VARIABLE; > + req_data->data_size = buffer_size; > + req_data->name_offset = name_offs; > + req_data->name_size = name_length * sizeof(*name); > + req_data->guid_offset = guid_offs; > + req_data->guid_size = sizeof(*guid); > + req_data->length = req_size; > + > + /* Copy request parameters. */ > + ucs2_strlcpy(((void *)req_data) + req_data->name_offset, name, name_length); > + memcpy(((void *)req_data) + req_data->guid_offset, guid, req_data->guid_size); > + > + /* Perform SCM call. */ > + status = qcom_scm_qseecom_app_send(qcuefi->client, req_data, req_size, rsp_data, rsp_size); > + > + /* Check for errors and validate. */ > + if (status) { > + efi_status = EFI_DEVICE_ERROR; > + goto out_free; > + } > + > + if (rsp_data->command_id != QSEE_CMD_UEFI_GET_VARIABLE) { > + efi_status = EFI_DEVICE_ERROR; > + goto out_free; > + } > + > + if (rsp_data->length < sizeof(*rsp_data)) { > + efi_status = EFI_DEVICE_ERROR; > + goto out_free; > + } > + > + if (rsp_data->status) { > + dev_dbg(qcuefi_dev(qcuefi), "%s: uefisecapp error: 0x%x\n", > + __func__, rsp_data->status); > + efi_status = qsee_uefi_status_to_efi(rsp_data->status); > + > + /* Update size and attributes in case buffer is too small. */ > + if (efi_status == EFI_BUFFER_TOO_SMALL) { > + *data_size = rsp_data->data_size; > + if (attributes) > + *attributes = rsp_data->attributes; > + } > + > + goto out_free; > + } > + > + if (rsp_data->length > rsp_size) { > + efi_status = EFI_DEVICE_ERROR; > + goto out_free; > + } > + > + if (rsp_data->data_offset + rsp_data->data_size > rsp_data->length) { > + efi_status = EFI_DEVICE_ERROR; > + goto out_free; > + } > + > + /* Set attributes and data size even if buffer is too small. */ > + *data_size = rsp_data->data_size; > + if (attributes) > + *attributes = rsp_data->attributes; > + > + /* > + * If we have a buffer size of zero and no buffer, just return > + * attributes and required size. > + */ > + if (buffer_size == 0 && !data) { > + efi_status = EFI_SUCCESS; > + goto out_free; > + } > + > + /* Validate output buffer size. */ > + if (buffer_size < rsp_data->data_size) { > + efi_status = EFI_BUFFER_TOO_SMALL; > + goto out_free; > + } > + > + /* Copy to output buffer. Note: We're guaranteed to have one at this point. */ > + memcpy(data, ((void *)rsp_data) + rsp_data->data_offset, rsp_data->data_size); > + > +out_free: > + kfree(rsp_data); > +out_free_req: > + kfree(req_data); > +out: > + return efi_status; > +} The above reads very easily as it stands, but generally we try to avoid adding comments inside functions unless doing something unexpected or similar. Comments like "Allocate response buffer" and "Perform SCM call" should not be needed as it should be clear from the code (given apt names of variables and function which you already seem to have chosen). > +/* -- Global efivar interface. ---------------------------------------------- */ > + > +static struct qcuefi_client *__qcuefi; > +static DEFINE_MUTEX(__qcuefi_lock); > + > +static int qcuefi_set_reference(struct qcuefi_client *qcuefi) > +{ > + mutex_lock(&__qcuefi_lock); > + > + if (qcuefi && __qcuefi) { > + mutex_unlock(&__qcuefi_lock); > + return -EEXIST; > + } > + > + __qcuefi = qcuefi; > + > + mutex_unlock(&__qcuefi_lock); > + return 0; > +} > + > +static struct qcuefi_client *qcuefi_acquire(void) > + __acquires(&__qcuefi_lock) I noticed that someone told you to add sparse annotation here but that was not correct. The mutex implementation does not use sparse annotation so this actually introduces sparse warnings such as: /home/johan/work/linaro/src/linux/drivers/firmware/qcom_qseecom_uefisecapp.c:741:29: warning: context imbalance in 'qcuefi_acquire' - wrong count at exit Just drop the annotation again. > +{ > + mutex_lock(&__qcuefi_lock); > + return __qcuefi; > +} > + > +static void qcuefi_release(void) > + __releases(&__qcuefi_lock) > +{ > + mutex_unlock(&__qcuefi_lock); > +} > +static int qcom_uefisecapp_probe(struct auxiliary_device *aux_dev, > + const struct auxiliary_device_id *aux_dev_id) > +{ > + struct qcuefi_client *qcuefi; > + int status; > + > + /* Allocate driver data. */ As mentioned above, I suggest dropping comments like this throughout. > + qcuefi = devm_kzalloc(&aux_dev->dev, sizeof(*qcuefi), GFP_KERNEL); > + if (!qcuefi) > + return -ENOMEM; > + > + qcuefi->client = container_of(aux_dev, struct qseecom_client, aux_dev); > + > + /* Register global reference. */ > + auxiliary_set_drvdata(aux_dev, qcuefi); > + status = qcuefi_set_reference(qcuefi); > + if (status) > + return status; > + > + /* Register efivar ops. */ > + status = efivars_register(&qcuefi->efivars, &qcom_efivar_ops); > + if (status) > + qcuefi_set_reference(NULL); > + > + return status; > +} Johan
First off, sorry again for the long delay and thanks for being patient with me (and for the review of course). I'm finally getting back to finding some time for Linux things again, so I think I've mostly settled in by now. On 6/28/23 13:20, Johan Hovold wrote: > On Mon, May 29, 2023 at 01:03:49AM +0200, Maximilian Luz wrote: >> When setting-up the IRQ goes wrong, the __scm pointer currently remains >> set even though we fail to probe the driver successfully. Due to this, >> access to __scm may go wrong since associated resources (clocks, ...) >> have been released. Therefore, clear the __scm pointer when setting-up >> the IRQ fails. >> >> Fixes: 6bf325992236 ("firmware: qcom: scm: Add wait-queue handling logic") >> Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com> >> --- >> >> Patch introduced in v4 >> >> --- >> drivers/firmware/qcom_scm.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c >> index fde33acd46b7..d0070b833889 100644 >> --- a/drivers/firmware/qcom_scm.c >> +++ b/drivers/firmware/qcom_scm.c >> @@ -1488,8 +1488,10 @@ static int qcom_scm_probe(struct platform_device *pdev) >> } else { >> ret = devm_request_threaded_irq(__scm->dev, irq, NULL, qcom_scm_irq_handler, >> IRQF_ONESHOT, "qcom-scm", __scm); >> - if (ret < 0) >> + if (ret < 0) { >> + __scm = NULL; > > This looks fragile at best. Clients use qcom_scm_is_available() to see > if __scm is available and do not expect it to go away once it is live. Hmm, you're right. The whole situation is probably not ideal and that fix is really just a bad band-aid. > It looks like you can hold off on initialising __scm until you've > requested the interrupt, either by using IRQ_NOAUTOEN or fixing > qcom_scm_waitq_wakeup() so that it doesn't use __scm directly. > > That would also take care of the previous branch which may also leave > __scm set after the structure itself has been released on errors. Agreed. > You'll have similar problems when registering qseecom which currently > depend on __scm being set, though. Clearing the pointer in that case is > clearly broken as you currently rely on devres for deregistering the aux > clients on errors (i.e. the clients using __scm are still registered > when you clear the pointer in patch 3/4). Oh right, I hadn't thought of that. I'll have to rework that. >> return dev_err_probe(scm->dev, ret, "Failed to request qcom-scm irq\n"); >> + } >> } >> >> __get_convention(); > > Johan
On 6/29/23 14:12, Johan Hovold wrote: > On Mon, May 29, 2023 at 01:03:51AM +0200, Maximilian Luz wrote: >> On platforms using the Qualcomm UEFI Secure Application (uefisecapp), >> EFI variables cannot be accessed via the standard interface in EFI >> runtime mode. The respective functions return EFI_UNSUPPORTED. On these >> platforms, we instead need to talk to uefisecapp. This commit provides >> support for this and registers the respective efivars operations to >> access EFI variables from the kernel. >> >> Communication with uefisecapp follows the Qualcomm QSEECOM / Secure OS >> conventions via the respective SCM call interface. This is also the >> reason why variable access works normally while boot services are >> active. During this time, said SCM interface is managed by the boot >> services. When calling ExitBootServices(), the ownership is transferred >> to the kernel. Therefore, UEFI must not use that interface itself (as >> multiple parties accessing this interface at the same time may lead to >> complications) and cannot access variables for us. >> >> Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com> >> --- >> >> Changes in v4: >> - Adapt to changes in DMA allocation in patch 3. >> - Rework alignment: Use native alignment of types instead of a general >> 8 byte alignment. While the windows driver uses 8 byte alignment for >> GUIDs, the native (4 byte) alignment seems to work fine here. >> - Add a helper macro for determining size and layout of request and >> response buffers, taking care of proper alignment. >> - Implement support for EFI's query_variable_info() call, which is now >> supported by the kernel (and expected by efivarfs). >> - Move UCS-2 string helpers to lib/ucs2_string.c > >> +config QCOM_QSEECOM_UEFISECAPP >> + tristate "Qualcomm SEE UEFI Secure App client driver" >> + select QCOM_SCM >> + select QCOM_SCM_QSEECOM > > This should just be > > depends on QCOM_SCM_QSEECOM > > Using select should generally be avoided. Okay. >> + depends on EFI >> + help >> + Various Qualcomm SoCs do not allow direct access to EFI variables. >> + Instead, these need to be accessed via the UEFI Secure Application >> + (uefisecapp), residing in the Secure Execution Environment (SEE). >> + >> + This module provides a client driver for uefisecapp, installing efivar >> + operations to allow the kernel accessing EFI variables, and via that also >> + provide user-space with access to EFI variables via efivarfs. >> + >> + Select M or Y here to provide access to EFI variables on the >> + aforementioned platforms. > > We still have not really sorted how best to handle modular efivars > implementations as userspace may try to mount efivarfs before the module > has been loaded... > > Perhaps require it to be built-in for now? Sure, I can do that. >> + >> config SYSFB >> bool >> select BOOT_VESA_SUPPORT > >> +/* -- Alighment helpers ----------------------------------------------------- */ > > typo: Alignment Oh, thanks for spotting that. >> + >> +/* >> + * Helper macro to ensure proper alignment of types (fields and arrays) when >> + * stored in some (contiguous) buffer. >> + * >> + * Note: The driver from which this one has been reverse-engineered expects an >> + * alignment of 8 bytes (64 bits) for GUIDs. Our definition of efi_guid_t, >> + * however, has an alignment of 4 byte (32 bits). So far, this seems to work >> + * fine here. See also the comment on the typedef of efi_guid_t. >> + */ >> +#define qcuefi_buf_align_fields(fields...) \ >> + ({ \ >> + size_t __len = 0; \ >> + fields \ >> + __len; \ >> + }) >> + >> +#define __field_impl(size, align, offset) \ >> + ({ \ >> + size_t *__offset = (offset); \ >> + size_t __aligned; \ >> + \ >> + __aligned = ALIGN(__len, align); \ >> + __len = __aligned + (size); \ >> + \ >> + if (__offset) \ >> + *__offset = __aligned; \ >> + }); >> + >> +#define __array_offs(type, count, offset) \ >> + __field_impl(sizeof(type) * (count), __alignof__(type), offset) >> + >> +#define __array(type, count) __array_offs(type, count, NULL) >> +#define __field_offs(type, offset) __array_offs(type, 1, offset) >> +#define __field(type) __array_offs(type, 1, NULL) > > Heh. This is some nice macro magic. :) > >> +static efi_status_t qsee_uefi_get_variable(struct qcuefi_client *qcuefi, const efi_char16_t *name, >> + const efi_guid_t *guid, u32 *attributes, >> + unsigned long *data_size, void *data) >> +{ >> + struct qsee_req_uefi_get_variable *req_data; >> + struct qsee_rsp_uefi_get_variable *rsp_data; >> + unsigned long buffer_size = *data_size; >> + efi_status_t efi_status = EFI_SUCCESS; >> + unsigned long name_length; >> + size_t guid_offs; >> + size_t name_offs; >> + size_t req_size; >> + size_t rsp_size; >> + int status; >> + >> + /* Validation: We need a name and GUID. */ >> + if (!name || !guid) >> + return EFI_INVALID_PARAMETER; >> + >> + /* Get length of name and ensure that the name is not truncated. */ >> + name_length = ucs2_strnlen(name, QSEE_MAX_NAME_LEN) + 1; >> + if (name_length > QSEE_MAX_NAME_LEN) >> + return EFI_INVALID_PARAMETER; >> + >> + /* Validation: We need a buffer if the buffer_size is nonzero. */ >> + if (buffer_size && !data) >> + return EFI_INVALID_PARAMETER; >> + >> + /* Compute required buffer size for request. */ >> + req_size = qcuefi_buf_align_fields( >> + __field(*req_data) >> + __array_offs(*name, name_length, &name_offs) >> + __field_offs(*guid, &guid_offs) >> + ); >> + >> + /* Compute required buffer size for response. */ >> + rsp_size = qcuefi_buf_align_fields( >> + __field(*rsp_data) >> + __array(u8, buffer_size) >> + ); >> + >> + /* Allocate request buffer. */ >> + req_data = kzalloc(req_size, GFP_KERNEL); >> + if (!req_data) { >> + efi_status = EFI_OUT_OF_RESOURCES; >> + goto out; >> + } >> + >> + /* Allocate response buffer. */ >> + rsp_data = kzalloc(rsp_size, GFP_KERNEL); >> + if (!rsp_data) { >> + efi_status = EFI_OUT_OF_RESOURCES; >> + goto out_free_req; >> + } >> + >> + /* Set up request data. */ >> + req_data->command_id = QSEE_CMD_UEFI_GET_VARIABLE; >> + req_data->data_size = buffer_size; >> + req_data->name_offset = name_offs; >> + req_data->name_size = name_length * sizeof(*name); >> + req_data->guid_offset = guid_offs; >> + req_data->guid_size = sizeof(*guid); >> + req_data->length = req_size; >> + >> + /* Copy request parameters. */ >> + ucs2_strlcpy(((void *)req_data) + req_data->name_offset, name, name_length); >> + memcpy(((void *)req_data) + req_data->guid_offset, guid, req_data->guid_size); >> + >> + /* Perform SCM call. */ >> + status = qcom_scm_qseecom_app_send(qcuefi->client, req_data, req_size, rsp_data, rsp_size); >> + >> + /* Check for errors and validate. */ >> + if (status) { >> + efi_status = EFI_DEVICE_ERROR; >> + goto out_free; >> + } >> + >> + if (rsp_data->command_id != QSEE_CMD_UEFI_GET_VARIABLE) { >> + efi_status = EFI_DEVICE_ERROR; >> + goto out_free; >> + } >> + >> + if (rsp_data->length < sizeof(*rsp_data)) { >> + efi_status = EFI_DEVICE_ERROR; >> + goto out_free; >> + } >> + >> + if (rsp_data->status) { >> + dev_dbg(qcuefi_dev(qcuefi), "%s: uefisecapp error: 0x%x\n", >> + __func__, rsp_data->status); >> + efi_status = qsee_uefi_status_to_efi(rsp_data->status); >> + >> + /* Update size and attributes in case buffer is too small. */ >> + if (efi_status == EFI_BUFFER_TOO_SMALL) { >> + *data_size = rsp_data->data_size; >> + if (attributes) >> + *attributes = rsp_data->attributes; >> + } >> + >> + goto out_free; >> + } >> + >> + if (rsp_data->length > rsp_size) { >> + efi_status = EFI_DEVICE_ERROR; >> + goto out_free; >> + } >> + >> + if (rsp_data->data_offset + rsp_data->data_size > rsp_data->length) { >> + efi_status = EFI_DEVICE_ERROR; >> + goto out_free; >> + } >> + >> + /* Set attributes and data size even if buffer is too small. */ >> + *data_size = rsp_data->data_size; >> + if (attributes) >> + *attributes = rsp_data->attributes; >> + >> + /* >> + * If we have a buffer size of zero and no buffer, just return >> + * attributes and required size. >> + */ >> + if (buffer_size == 0 && !data) { >> + efi_status = EFI_SUCCESS; >> + goto out_free; >> + } >> + >> + /* Validate output buffer size. */ >> + if (buffer_size < rsp_data->data_size) { >> + efi_status = EFI_BUFFER_TOO_SMALL; >> + goto out_free; >> + } >> + >> + /* Copy to output buffer. Note: We're guaranteed to have one at this point. */ >> + memcpy(data, ((void *)rsp_data) + rsp_data->data_offset, rsp_data->data_size); >> + >> +out_free: >> + kfree(rsp_data); >> +out_free_req: >> + kfree(req_data); >> +out: >> + return efi_status; >> +} > > The above reads very easily as it stands, but generally we try to avoid > adding comments inside functions unless doing something unexpected or > similar. > > Comments like "Allocate response buffer" and "Perform SCM call" should > not be needed as it should be clear from the code (given apt names of > variables and function which you already seem to have chosen). Okay, I'll drop those. >> +/* -- Global efivar interface. ---------------------------------------------- */ >> + >> +static struct qcuefi_client *__qcuefi; >> +static DEFINE_MUTEX(__qcuefi_lock); >> + >> +static int qcuefi_set_reference(struct qcuefi_client *qcuefi) >> +{ >> + mutex_lock(&__qcuefi_lock); >> + >> + if (qcuefi && __qcuefi) { >> + mutex_unlock(&__qcuefi_lock); >> + return -EEXIST; >> + } >> + >> + __qcuefi = qcuefi; >> + >> + mutex_unlock(&__qcuefi_lock); >> + return 0; >> +} >> + >> +static struct qcuefi_client *qcuefi_acquire(void) >> + __acquires(&__qcuefi_lock) > > I noticed that someone told you to add sparse annotation here but that > was not correct. > > The mutex implementation does not use sparse annotation so this actually > introduces sparse warnings such as: > > /home/johan/work/linaro/src/linux/drivers/firmware/qcom_qseecom_uefisecapp.c:741:29: warning: context imbalance in 'qcuefi_acquire' - wrong count at exit > > Just drop the annotation again. Sure, will do. I might also want to look into actually running sparse at some point I guess... >> +{ >> + mutex_lock(&__qcuefi_lock); >> + return __qcuefi; >> +} >> + >> +static void qcuefi_release(void) >> + __releases(&__qcuefi_lock) >> +{ >> + mutex_unlock(&__qcuefi_lock); >> +} > >> +static int qcom_uefisecapp_probe(struct auxiliary_device *aux_dev, >> + const struct auxiliary_device_id *aux_dev_id) >> +{ >> + struct qcuefi_client *qcuefi; >> + int status; >> + >> + /* Allocate driver data. */ > > As mentioned above, I suggest dropping comments like this throughout. Sure. >> + qcuefi = devm_kzalloc(&aux_dev->dev, sizeof(*qcuefi), GFP_KERNEL); >> + if (!qcuefi) >> + return -ENOMEM; >> + >> + qcuefi->client = container_of(aux_dev, struct qseecom_client, aux_dev); >> + >> + /* Register global reference. */ >> + auxiliary_set_drvdata(aux_dev, qcuefi); >> + status = qcuefi_set_reference(qcuefi); >> + if (status) >> + return status; >> + >> + /* Register efivar ops. */ >> + status = efivars_register(&qcuefi->efivars, &qcom_efivar_ops); >> + if (status) >> + qcuefi_set_reference(NULL); >> + >> + return status; >> +} Again, thanks for the review and the patience. I'll try to prepare a new version over the weekend. Regards Max
This series adds basic support for the QSEECOM interface used to communicate with secure applications running in the TrustZone on certain Qualcomm devices. In addition to that, it also provides a driver for "uefisecapp", the secure application managing access to UEFI variables on such platforms. For a more detailed description, see the blurb of v1. Previous versions: - V3: https://lore.kernel.org/lkml/20230305022119.1331495-4-luzmaximilian@gmail.com/t/ - V2: https://lore.kernel.org/lkml/20230127184650.756795-1-luzmaximilian@gmail.com/ - V1: https://lore.kernel.org/lkml/20220723224949.1089973-1-luzmaximilian@gmail.com/ Patch 4 of this series depends on commit d86ff3333cb1 ("efivarfs: expose used and total size") from the "next" branch of https://git.kernel.org/pub/scm/linux/kernel/git/efi/efi.git Changes in v4: - Integrate the QSEECOM interface into qcom_scm.c instead of instantiating a custom device and requiring device-tree bindings for it. With that, drop the respective patches exporting SCM call functions from qcom_scm.c and the DT bindings. - Restructure management of DMA memory and move DMA mapping entirely into the app_send() command, removing the need for DMA handling in app client drivers. - Add support for EFI's query_variable_info() call. - Move UCS-2 string helpers to lib/ucs2_string.c (introduces patch 1). - Add fix for related cleanup-issue in qcom_scm.c (introduces patch 2). (Refer to individual patches for more details.) Changes in v3: - Fix doc comment in qcom_scm.c - Rebase on top of latest changes to qcom_scm. Changes in v2: - Bind the qseecom interface to a device. - Establish a device link between the new qseecom device and the SCM device to ensure proper PM and remove ordering. - Remove the compatible for uefisecapp. Instead, introduce a compatible for the qseecom device. This directly reflects ACPI tables and the QCOM0476 device described therein, which is responsible for the secure app / qseecom interface (i.e., the same purpose). Client devices representing apps handled by the kernel (such as uefisecapp) are now directly instantiated by the qseecom driver, based on the respective platform-specific compatible. - Rename the base name (qctree -> qseecom) to allow differentiation between old (qseecom) and new (smcinvoke) interfaces to the trusted execution environment. This directly reflects downstream naming by Qualcomm. Maximilian Luz (4): lib/ucs2_string: Add UCS-2 strlcpy function firmware: qcom_scm: Clear scm pointer on probe failure firmware: qcom_scm: Add support for Qualcomm Secure Execution Environment SCM interface firmware: Add support for Qualcomm UEFI Secure Application MAINTAINERS | 6 + drivers/firmware/Kconfig | 33 + drivers/firmware/Makefile | 1 + drivers/firmware/qcom_qseecom_uefisecapp.c | 885 +++++++++++++++++++++ drivers/firmware/qcom_scm.c | 419 +++++++++- include/linux/firmware/qcom/qcom_scm.h | 27 + include/linux/ucs2_string.h | 1 + lib/ucs2_string.c | 16 + 8 files changed, 1387 insertions(+), 1 deletion(-) create mode 100644 drivers/firmware/qcom_qseecom_uefisecapp.c