linux-kernel - Re: [RFC PATCH v3 15/16] cxl/mem: Add limited Get Log command (0401h)

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:   Thu, 14 Jan 2021 18:08:26 +0000
From:   Jonathan Cameron <Jonathan.Cameron@...wei.com>
To:     Ben Widawsky <ben.widawsky@...el.com>
CC:     <linux-cxl@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <linux-pci@...r.kernel.org>,
        "linux-acpi@...r.kernel.org, Ira Weiny" <ira.weiny@...el.com>,
        Dan Williams <dan.j.williams@...el.com>,
        "Vishal Verma" <vishal.l.verma@...el.com>,
        "Kelley, Sean V" <sean.v.kelley@...el.com>,
        Rafael Wysocki <rafael.j.wysocki@...el.com>,
        "Bjorn Helgaas" <helgaas@...nel.org>,
        Jon Masters <jcm@...masters.org>,
        Chris Browy <cbrowy@...ry-design.com>,
        Randy Dunlap <rdunlap@...radead.org>,
        "Christoph Hellwig" <hch@...radead.org>,
        <daniel.lll@...baba-inc.com>
Subject: Re: [RFC PATCH v3 15/16] cxl/mem: Add limited Get Log command
 (0401h)

On Mon, 11 Jan 2021 14:51:20 -0800
Ben Widawsky <ben.widawsky@...el.com> wrote:

> The Get Log command returns the actual log entries that are advertised
> via the Get Supported Logs command (0400h). CXL device logs are selected
> by UUID which is part of the CXL spec. Because the driver tries to
> sanitize what is sent to hardware, there becomes a need to restrict the
> types of logs which can be accessed by userspace. For example, the
> vendor specific log might only be consumable by proprietary, or offline
> applications, and therefore a good candidate for userspace.
> 
> The current driver infrastructure does allow basic validation for all
> commands, but doesn't inspect any of the payload data. Along with Get
> Log support comes new infrastructure to add a hook for payload
> validation. This infrastructure is used to filter out the CEL UUID,
> which the userspace driver doesn't have business knowing, and taints on
> invalid UUIDs being sent to hardware.
> 
> Signed-off-by: Ben Widawsky <ben.widawsky@...el.com>

Just a minor question for this one.

Thanks, J
...                                              \
> @@ -515,6 +529,15 @@ static int handle_mailbox_cmd_from_user(struct cxl_memdev *cxlmd,
>  	int rc;
>  
>  	if (cmd->info.size_in) {
> +		if (cmd->validate_payload) {
> +			rc = cmd->validate_payload(u64_to_user_ptr(in_payload),
> +						   cmd->info.size_in);

Is it worth moving this out of the region in which we hold the mbox?
(after fixing the bug that I think means we don't actually hold it at this point)

Perhaps not, but it does feel odd to do validation under the lock.


> +			if (rc) {
> +				cxl_mem_mbox_put(cxlmd->cxlm);
> +				return -EFAULT;
> +			}
> +		}
> +
>  		/*
>  		 * Directly copy the userspace payload into the hardware. UAPI
>  		 * states that the buffer must already be little endian.