| 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
| ||
|
Message-ID: <20250304170808.0000451d@huawei.com>
Date: Tue, 4 Mar 2025 17:08:08 +0800
From: Jonathan Cameron <Jonathan.Cameron@...wei.com>
To: Shannon Nelson <shannon.nelson@....com>
CC: <jgg@...dia.com>, <andrew.gospodarek@...adcom.com>,
<aron.silverton@...cle.com>, <dan.j.williams@...el.com>,
<daniel.vetter@...ll.ch>, <dave.jiang@...el.com>, <dsahern@...nel.org>,
<gregkh@...uxfoundation.org>, <hch@...radead.org>, <itayavr@...dia.com>,
<jiri@...dia.com>, <kuba@...nel.org>, <lbloch@...dia.com>,
<leonro@...dia.com>, <linux-cxl@...r.kernel.org>,
<linux-rdma@...r.kernel.org>, <netdev@...r.kernel.org>, <saeedm@...dia.com>,
<brett.creeley@....com>
Subject: Re: [PATCH v2 5/6] pds_fwctl: add rpc and query support
On Fri, 28 Feb 2025 17:35:53 -0800
Shannon Nelson <shannon.nelson@....com> wrote:
> From: Brett Creeley <brett.creeley@....com>
>
> The pds_fwctl driver doesn't know what RPC operations are available
> in the firmware, so also doesn't know what scope they might have. The
> userland utility supplies the firmware "endpoint" and "operation" id values
> and this driver queries the firmware for endpoints and their available
> operations. The operation descriptions include the scope information
> which the driver uses for scope testing.
>
> Signed-off-by: Brett Creeley <brett.creeley@....com>
> Signed-off-by: Shannon Nelson <shannon.nelson@....com>
Hi
A few minor suggestions inline,
Jonathan
> +static int pdsfc_validate_rpc(struct pdsfc_dev *pdsfc,
> + struct fwctl_rpc_pds *rpc,
> + enum fwctl_rpc_scope scope)
> +{
> + struct pds_fwctl_query_data_operation *op_entry;
> + struct pdsfc_rpc_endpoint_info *ep_info = NULL;
> + struct device *dev = &pdsfc->fwctl.dev;
> + int i;
> +
> + /* validate rpc in_len & out_len based
> + * on ident.max_req_sz & max_resp_sz
> + */
> + if (rpc->in.len > pdsfc->ident.max_req_sz) {
> + dev_err(dev, "Invalid request size %u, max %u\n",
> + rpc->in.len, pdsfc->ident.max_req_sz);
> + return -EINVAL;
> + }
> +
> + if (rpc->out.len > pdsfc->ident.max_resp_sz) {
> + dev_err(dev, "Invalid response size %u, max %u\n",
> + rpc->out.len, pdsfc->ident.max_resp_sz);
> + return -EINVAL;
> + }
> +
> + for (i = 0; i < pdsfc->endpoints->num_entries; i++) {
> + if (pdsfc->endpoint_info[i].endpoint == rpc->in.ep) {
> + ep_info = &pdsfc->endpoint_info[i];
> + break;
> + }
> + }
> + if (!ep_info) {
> + dev_err(dev, "Invalid endpoint %d\n", rpc->in.ep);
> + return -EINVAL;
> + }
> +
> + /* query and cache this endpoint's operations */
> + mutex_lock(&ep_info->lock);
> + if (!ep_info->operations) {
> + ep_info->operations = pdsfc_get_operations(pdsfc,
> + &ep_info->operations_pa,
> + rpc->in.ep);
> + if (!ep_info->operations) {
> + mutex_unlock(&ep_info->lock);
> + dev_err(dev, "Failed to allocate operations list\n");
> + return -ENOMEM;
> + }
> + }
> + mutex_unlock(&ep_info->lock);
> +
> + /* reject unsupported and/or out of scope commands */
> + op_entry = (struct pds_fwctl_query_data_operation *)ep_info->operations->entries;
> + for (i = 0; i < ep_info->operations->num_entries; i++) {
> + if (PDS_FWCTL_RPC_OPCODE_CMP(rpc->in.op, op_entry[i].id)) {
> + if (scope < op_entry[i].scope)
> + return -EPERM;
> + return 0;
> + }
> + }
> +
> + dev_err(dev, "Invalid operation %d for endpoint %d\n", rpc->in.op, rpc->in.ep);
Perhaps a little noisy as I think userspace can trigger this easily. dev_dbg()
might be better. -EINVAL should be all userspace needs under most circumstances.
> +
> + return -EINVAL;
> +}
> +
> static void *pdsfc_fw_rpc(struct fwctl_uctx *uctx, enum fwctl_rpc_scope scope,
> void *in, size_t in_len, size_t *out_len)
> {
> - return NULL;
> + struct pdsfc_dev *pdsfc = container_of(uctx->fwctl, struct pdsfc_dev, fwctl);
> + struct fwctl_rpc_pds *rpc = (struct fwctl_rpc_pds *)in;
in is a void * and the C spec always allows implicit conversion for those
so no cast here.
> + struct device *dev = &uctx->fwctl->dev;
> + union pds_core_adminq_comp comp = {0};
> + dma_addr_t out_payload_dma_addr = 0;
> + dma_addr_t in_payload_dma_addr = 0;
> + union pds_core_adminq_cmd cmd;
> + void *out_payload = NULL;
> + void *in_payload = NULL;
> + void *out = NULL;
> + int err;
> +
> + err = pdsfc_validate_rpc(pdsfc, rpc, scope);
> + if (err) {
> + dev_err(dev, "Invalid RPC request\n");
> + return ERR_PTR(err);
> + }
> +
> + if (rpc->in.len > 0) {
> + in_payload = kzalloc(rpc->in.len, GFP_KERNEL);
> + if (!in_payload) {
> + dev_err(dev, "Failed to allocate in_payload\n");
> + out = ERR_PTR(-ENOMEM);
> + goto done;
> + }
> +
> + if (copy_from_user(in_payload, u64_to_user_ptr(rpc->in.payload),
> + rpc->in.len)) {
> + dev_err(dev, "Failed to copy in_payload from user\n");
> + out = ERR_PTR(-EFAULT);
> + goto done;
> + }
> +
> + in_payload_dma_addr = dma_map_single(dev->parent, in_payload,
> + rpc->in.len, DMA_TO_DEVICE);
> + err = dma_mapping_error(dev->parent, in_payload_dma_addr);
> + if (err) {
> + dev_err(dev, "Failed to map in_payload\n");
> + in_payload_dma_addr = 0;
See below for comment on ordering and why ideally this zero setting should
not be needed in error path. Really small point though!
> + out = ERR_PTR(err);
> + goto done;
> + }
> + }
> +
> + if (rpc->out.len > 0) {
> + out_payload = kzalloc(rpc->out.len, GFP_KERNEL);
> + if (!out_payload) {
> + dev_err(dev, "Failed to allocate out_payload\n");
> + out = ERR_PTR(-ENOMEM);
> + goto done;
> + }
> +
> + out_payload_dma_addr = dma_map_single(dev->parent, out_payload,
> + rpc->out.len, DMA_FROM_DEVICE);
> + err = dma_mapping_error(dev->parent, out_payload_dma_addr);
> + if (err) {
> + dev_err(dev, "Failed to map out_payload\n");
> + out_payload_dma_addr = 0;
As above. Slight ordering tweaks and more labels would avoid need
to zero this on error as we would never use it again.
> + out = ERR_PTR(err);
> + goto done;
> + }
> + }
> +
> + cmd = (union pds_core_adminq_cmd) {
> + .fwctl_rpc = {
> + .opcode = PDS_FWCTL_CMD_RPC,
> + .flags = PDS_FWCTL_RPC_IND_REQ | PDS_FWCTL_RPC_IND_RESP,
> + .ep = cpu_to_le32(rpc->in.ep),
> + .op = cpu_to_le32(rpc->in.op),
> + .req_pa = cpu_to_le64(in_payload_dma_addr),
> + .req_sz = cpu_to_le32(rpc->in.len),
> + .resp_pa = cpu_to_le64(out_payload_dma_addr),
> + .resp_sz = cpu_to_le32(rpc->out.len),
> + }
> + };
> +
> + err = pds_client_adminq_cmd(pdsfc->padev, &cmd, sizeof(cmd), &comp, 0);
> + if (err) {
> + dev_err(dev, "%s: ep %d op %x req_pa %llx req_sz %d req_sg %d resp_pa %llx resp_sz %d resp_sg %d err %d\n",
> + __func__, rpc->in.ep, rpc->in.op,
> + cmd.fwctl_rpc.req_pa, cmd.fwctl_rpc.req_sz, cmd.fwctl_rpc.req_sg_elems,
> + cmd.fwctl_rpc.resp_pa, cmd.fwctl_rpc.resp_sz, cmd.fwctl_rpc.resp_sg_elems,
> + err);
> + out = ERR_PTR(err);
> + goto done;
> + }
> +
> + dynamic_hex_dump("out ", DUMP_PREFIX_OFFSET, 16, 1, out_payload, rpc->out.len, true);
> +
> + if (copy_to_user(u64_to_user_ptr(rpc->out.payload), out_payload, rpc->out.len)) {
> + dev_err(dev, "Failed to copy out_payload to user\n");
> + out = ERR_PTR(-EFAULT);
> + goto done;
> + }
> +
> + rpc->out.retval = le32_to_cpu(comp.fwctl_rpc.err);
> + *out_len = in_len;
> + out = in;
> +
> +done:
> + if (in_payload_dma_addr)
> + dma_unmap_single(dev->parent, in_payload_dma_addr,
> + rpc->in.len, DMA_TO_DEVICE);
> +
> + if (out_payload_dma_addr)
Can we do these in reverse order of the setup path?
It obviously makes limited difference in practice.
With them in strict reverse order you could avoid need to
zero out_payload_dma_addr and similar in error paths by
using multiple labels.
> + dma_unmap_single(dev->parent, out_payload_dma_addr,
> + rpc->out.len, DMA_FROM_DEVICE);
> +
> + kfree(in_payload);
> + kfree(out_payload);
> +
> + return out;
At least some cases are simplified if you do
if (err)
return ERR_PTR(err);
return in;
and handle all errors via err integer until here.
> }
> static const struct auxiliary_device_id pdsfc_id_table[] = {
> diff --git a/include/linux/pds/pds_adminq.h b/include/linux/pds/pds_adminq.h
> index ae6886ebc841..03bca2d27de0 100644
> --- a/include/linux/pds/pds_adminq.h
> +++ b/include/linux/pds/pds_adminq.h
> +/**
> + * struct pds_fwctl_query_data - query data structure
> + * @version: Version of the query data structure
> + * @rsvd: Word boundary padding
> + * @num_entries: Number of entries in the union
> + * @entries: Array of query data entries, depending on the entity type.
> + */
> +struct pds_fwctl_query_data {
> + u8 version;
> + u8 rsvd[3];
> + __le32 num_entries;
> + uint8_t entries[];
__counted_by_le() probably a nice to have here.
> +} __packed;
> +/**
> + * struct pds_sg_elem - Transmit scatter-gather (SG) descriptor element
> + * @addr: DMA address of SG element data buffer
> + * @len: Length of SG element data buffer, in bytes
> + * @rsvd: Word boundary padding
> + */
> +struct pds_sg_elem {
> + __le64 addr;
> + __le32 len;
> + __le16 rsvd[2];
__le32 rsvd; ? Or stick to u8 only.
> +} __packed;
> +
> +/**
> + * struct pds_fwctl_rpc_comp - Completion of a firmware control RPC.
> + * @status: Status of the command
> + * @rsvd: Word boundary padding
> + * @comp_index: Completion index of the command
> + * @err: Error code, if any, from the RPC.
Odd mix of full stop and not. Make it more consistent.
> + * @resp_sz: Size of the response.
> + * @rsvd2: Word boundary padding
words changing size in one structure? Just stick to "Reserved" for
the docs. Never any reason to explicitly talk about 'why' things
are reserved.
> + * @color: Color bit indicating the state of the completion.
> + */
> +struct pds_fwctl_rpc_comp {
> + u8 status;
> + u8 rsvd;
> + __le16 comp_index;
> + __le32 err;
> + __le32 resp_sz;
> + u8 rsvd2[3];
> + u8 color;
> +} __packed;
> +
> union pds_core_adminq_cmd {
> u8 opcode;
> u8 bytes[64];
> @@ -1291,6 +1474,8 @@ union pds_core_adminq_cmd {
>
> struct pds_fwctl_cmd fwctl;
> struct pds_fwctl_ident_cmd fwctl_ident;
> + struct pds_fwctl_rpc_cmd fwctl_rpc;
> + struct pds_fwctl_query_cmd fwctl_query;
> };
>
> union pds_core_adminq_comp {
> @@ -1320,6 +1505,8 @@ union pds_core_adminq_comp {
> struct pds_lm_dirty_status_comp lm_dirty_status;
>
> struct pds_fwctl_comp fwctl;
> + struct pds_fwctl_rpc_comp fwctl_rpc;
> + struct pds_fwctl_query_comp fwctl_query;
> };
>
> #ifndef __CHECKER__
> diff --git a/include/uapi/fwctl/pds.h b/include/uapi/fwctl/pds.h
> index a01b032cbdb1..da6cd2d1c6fa 100644
> --- a/include/uapi/fwctl/pds.h
> +++ b/include/uapi/fwctl/pds.h
> @@ -24,4 +24,20 @@ enum pds_fwctl_capabilities {
> PDS_FWCTL_QUERY_CAP = 0,
> PDS_FWCTL_SEND_CAP,
> };
> +
> +struct fwctl_rpc_pds {
> + struct {
> + __u32 op;
> + __u32 ep;
> + __u32 rsvd;
> + __u32 len;
> + __u64 payload;
> + } in;
> + struct {
> + __u32 retval;
> + __u32 rsvd[2];
> + __u32 len;
> + __u64 payload;
> + } out;
> +};
> #endif /* _UAPI_FWCTL_PDS_H_ */
Powered by blists - more mailing lists