| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 22 Oct 2021 23:05:55 +0300
From: Andy Shevchenko <andriy.shevchenko@...el.com>
To: Chen Yu <yu.c.chen@...el.com>
Cc: linux-acpi@...r.kernel.org,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
"Rafael J. Wysocki" <rafael@...nel.org>,
Ard Biesheuvel <ardb@...nel.org>, Len Brown <lenb@...nel.org>,
Ashok Raj <ashok.raj@...el.com>,
Mike Rapoport <rppt@...nel.org>,
Aubrey Li <aubrey.li@...el.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 2/4] drivers/acpi: Introduce Platform Firmware Runtime
Update device driver
On Sat, Oct 23, 2021 at 01:09:51AM +0800, Chen Yu wrote:
> Introduce the pfru_update driver which can be used for Platform Firmware
> Runtime code injection and driver update[1]. The user is expected to
> provide the update firmware in the form of capsule file, and pass it to
> the driver via ioctl. Then the driver would hand this capsule file to the
> Platform Firmware Runtime Update via the ACPI device _DSM method. At last
> the low level Management Mode would do the firmware update.
>
> The corresponding userspace tool and man page will be introduced at
> tools/power/acpi/pfru.
>
> [1] https://uefi.org/sites/default/files/resources/Intel_MM_OS_Interface_Spec_Rev100.pdf
Instead use Link: tag ?
...
> +enum cap_index {
> + CAP_STATUS_IDX,
> + CAP_UPDATE_IDX,
> + CAP_CODE_TYPE_IDX,
> + CAP_FW_VER_IDX,
> + CAP_CODE_RT_VER_IDX,
> + CAP_DRV_TYPE_IDX,
> + CAP_DRV_RT_VER_IDX,
> + CAP_DRV_SVN_IDX,
> + CAP_PLAT_ID_IDX,
> + CAP_OEM_ID_IDX,
> + CAP_OEM_INFO_IDX,
> + CAP_NR_IDX,
Is it terminator entry? Drop comma then. Same for all other similar cases.
> +};
...
> +static int get_image_type(efi_manage_capsule_image_header_t *img_hdr,
> + struct pfru_device *pfru_dev)
> +{
> + guid_t *image_type_id = &img_hdr->image_type_id;
> +
> + /* check whether this is a code injection or driver update */
> + if (guid_equal(image_type_id, &pfru_dev->code_uuid))
> + return CODE_INJECT_TYPE;
> + else if (guid_equal(image_type_id, &pfru_dev->drv_uuid))
> + return DRIVER_UPDATE_TYPE;
> + else
> + return -EINVAL;
In both cases redundant 'else'.
> +}
...
> +static int adjust_efi_size(efi_manage_capsule_image_header_t *img_hdr,
> + int size)
> +{
> + /*
> + * The (u64 hw_ins) was introduced in UEFI spec version 2,
> + * and (u64 capsule_support) was introduced in version 3.
> + * The size needs to be adjusted accordingly. That is to
> + * say, version 1 should subtract the size of hw_ins+capsule_support,
> + * and version 2 should sbstract the size of capsule_support.
> + */
> + size += sizeof(efi_manage_capsule_image_header_t);
> + switch (img_hdr->ver) {
> + case 1:
> + size -= 2 * sizeof(u64);
> + break;
> + case 2:
> + size -= sizeof(u64);
> + break;
> + default:
> + /* only support version 1 and 2 */
> + return -EINVAL;
> + }
> + return size;
Perhaps directly return this from the cases?
> +}
...
> +static long pfru_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> +{
> + struct pfru_update_cap_info cap;
> + struct pfru_device *pfru_dev;
> + void __user *p;
> + int ret = 0, rev;
return 0; (see below)?
> + pfru_dev = to_pfru_dev(file);
> + p = (void __user *)arg;
> +
> + switch (cmd) {
> + case PFRU_IOC_QUERY_CAP:
> + ret = query_capability(&cap, pfru_dev);
> + if (ret)
> + return ret;
> +
> + if (copy_to_user(p, &cap, sizeof(cap)))
> + return -EFAULT;
> +
> + break;
> + case PFRU_IOC_SET_REV:
> + if (copy_from_user(&rev, p, sizeof(unsigned int)))
> + return -EFAULT;
> +
> + if (!pfru_valid_revid(rev))
> + return -EINVAL;
> +
> + pfru_dev->rev_id = rev;
> + break;
> + case PFRU_IOC_STAGE:
> + ret = start_acpi_update(START_STAGE, pfru_dev);
> + break;
> + case PFRU_IOC_ACTIVATE:
> + ret = start_acpi_update(START_ACTIVATE, pfru_dev);
> + break;
> + case PFRU_IOC_STAGE_ACTIVATE:
> + ret = start_acpi_update(START_STAGE_ACTIVATE, pfru_dev);
> + break;
> + default:
> + ret = -ENOTTY;
> + break;
> + }
> + return ret;
You may return 0 here and directly return from the cases above.
> +}
...
> + /* map the communication buffer */
Comment style inconsistency. Compare to...
> + /* Check if the capsule header has a valid version number. */
...this one. However, here, since it's one line, no period is needed.
...
> +out:
> + devm_kfree(&pdev->dev, pfru_dev);
What is this? WHy?!
> +}
...
> +static int __init pfru_init(void)
> +{
> + return platform_driver_register(&acpi_pfru_driver);
> +}
> +
> +static void __exit pfru_exit(void)
> +{
> + platform_driver_unregister(&acpi_pfru_driver);
> +}
> +
> +module_init(pfru_init);
> +module_exit(pfru_exit);
NIH module_platform_driver().
...
> +#include <linux/uuid.h>
This is wrong. Please, use raw buffers.
...
Missed types.h.
...
> +struct pfru_payload_hdr {
> + __u32 sig;
> + __u32 hdr_version;
> + __u32 hdr_size;
> + __u32 hw_ver;
> + __u32 rt_ver;
> + uuid_t platform_id;
No way. Use __u8[16].
uuid_t is internal type for kernel.
> +};
...
> +struct pfru_update_cap_info {
> + enum pfru_dsm_status status;
> + __u32 update_cap;
> +
> + uuid_t code_type;
Ditto.
> + __u32 fw_version;
> + __u32 code_rt_version;
> + uuid_t drv_type;
Ditto.
> + __u32 drv_rt_version;
> + __u32 drv_svn;
> + uuid_t platform_id;
> + uuid_t oem_id;
Ditto.
> + char oem_info[];
> +};
...
> +struct pfru_updated_result {
> + enum pfru_dsm_status status;
> + enum pfru_dsm_status ext_status;
What?! Are you sure enum is the same size on all possible architectures?
> + __u64 low_auth_time;
> + __u64 high_auth_time;
> + __u64 low_exec_time;
> + __u64 high_exec_time;
> +};
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists