| 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: <a7050314-bcb3-4ec1-98f6-334019222235@amd.com>
Date: Wed, 5 Mar 2025 17:55:45 -0800
From: "Nelson, Shannon" <shannon.nelson@....com>
To: Jonathan Cameron <Jonathan.Cameron@...wei.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 3/4/2025 1:08 AM, Jonathan Cameron wrote:
> 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.
Sure
>
>> +
>> + 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.
Will fix
>
>> + 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.
Yeah, I'll reorder this with a couple more 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.
Yes.
>
>
>> +} __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.
I'll use u8
>
>
>> +} __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.
Sure
>
>> + * @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.
Sure
Thanks,
sln
>
>> + * @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