linux-kernel - Re: [PATCH v3 3/5] drivers/acpi: Introduce Platform Firmware Runtime Update device driver

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 21 Sep 2021 17:59:05 +0200
From:   Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To:     Chen Yu <yu.c.chen@...el.com>
Cc:     linux-acpi@...r.kernel.org, linux-kernel@...r.kernel.org,
        "Rafael J. Wysocki" <rafael@...nel.org>,
        Len Brown <len.brown@...el.com>,
        Dan Williams <dan.j.williams@...el.com>,
        Andy Shevchenko <andriy.shevchenko@...el.com>,
        Aubrey Li <aubrey.li@...el.com>,
        Ashok Raj <ashok.raj@...el.com>,
        Mike Rapoport <rppt@...nel.org>,
        Ard Biesheuvel <ardb@...nel.org>,
        Jonathan Corbet <corbet@....net>,
        Hans de Goede <hdegoede@...hat.com>,
        Ben Widawsky <ben.widawsky@...el.com>,
        linux-doc@...r.kernel.org
Subject: Re: [PATCH v3 3/5] drivers/acpi: Introduce Platform Firmware Runtime
 Update device driver

On Fri, Sep 17, 2021 at 12:02:18AM +0800, Chen Yu wrote:
> Introduce the pfru_update driver which can be used for Platform Firmware
> Runtime code injection and driver update. 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.
> 
> Signed-off-by: Chen Yu <yu.c.chen@...el.com>

Where is the userspace code that uses this ioctl and has tested it out
to verify it works properly?  A link to that in the changelog would be
great to have.

> +static void dump_update_result(struct pfru_updated_result *result)
> +{
> +	pr_debug("Update result:\n");
> +	pr_debug("Status:%d\n", result->status);
> +	pr_debug("Extended Status:%d\n", result->ext_status);
> +	pr_debug("Authentication Time Low:%lld\n", result->low_auth_time);
> +	pr_debug("Authentication Time High:%lld\n", result->high_auth_time);
> +	pr_debug("Execution Time Low:%lld\n", result->low_exec_time);
> +	pr_debug("Execution Time High:%lld\n", result->high_exec_time);

Why not dev_dbg()?  Same for all pr_* calls in this "driver".


> +static long pfru_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> +{
> +	void __user *p;
> +	int ret = 0, rev;
> +
> +	p = (void __user *)arg;
> +
> +	switch (cmd) {
> +	case PFRU_IOC_SET_REV:
> +		if (copy_from_user(&rev, p, sizeof(unsigned int)))
> +			return -EFAULT;
> +		if (!pfru_valid_revid(rev))
> +			return -EFAULT;
> +		pfru_dev->rev_id = rev;
> +		break;
> +	case PFRU_IOC_STAGE:
> +		ret = start_acpi_update(START_STAGE);
> +		break;
> +	case PFRU_IOC_ACTIVATE:
> +		ret = start_acpi_update(START_ACTIVATE);
> +		break;
> +	case PFRU_IOC_STAGE_ACTIVATE:
> +		ret = start_acpi_update(START_STAGE_ACTIVATE);
> +		break;
> +	default:
> +		ret = -ENOIOCTLCMD;

Wrong value :(



> +		break;
> +	}
> +
> +	return ret;
> +}
> +
> +#ifdef CONFIG_COMPAT
> +static long compat_pfru_ioctl(struct file *filep, unsigned int cmd,
> +			      unsigned long arg)
> +{
> +	return pfru_ioctl(filep, cmd, arg);
> +}
> +#endif

Why is this compat ioctl needed at all?

> +static struct miscdevice pfru_misc_dev = {
> +	.minor = MISC_DYNAMIC_MINOR,
> +	.name = "pfru_update",
> +	.nodename = "pfru/update",

Why is this in a subdirectory?  What requires this?  Why not just
"pfru"?

thanks,

greg k-h