netdev - Re: [PATCH v2 5/6] pds_fwctl: add rpc and query support

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]

Message-ID: <20250304195233.GU133783@nvidia.com>
Date: Tue, 4 Mar 2025 15:52:33 -0400
From: Jason Gunthorpe <jgg@...dia.com>
To: Shannon Nelson <shannon.nelson@....com>
Cc: 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, Jonathan.Cameron@...wei.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, Feb 28, 2025 at 05:35:53PM -0800, Shannon Nelson wrote:

> +	/* 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;

Oh I see, you are doing the scope like CXL with a "command intent's
report". That is a good idea.

> +	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;
> +		}

This wasn't really the intention to have a payload pointer inside the
existing payload pointer, is it the same issue as CXL where you wanted
a header?

I see your FW API also can't take a scatterlist, so it makes sense it
needs to be linearized like this.

I don't think it makes a difference to have the indirection or not, it
would be a bunch of stuff to keep the header seperate from the payload
and then still also end up with memcpy in the driver, so leave it.

> +#define PDS_FWCTL_RPC_OPCODE_CMD_SHIFT	0
> +#define PDS_FWCTL_RPC_OPCODE_CMD_MASK	GENMASK(15, PDS_FWCTL_RPC_OPCODE_CMD_SHIFT)
> +#define PDS_FWCTL_RPC_OPCODE_VER_SHIFT	16
> +#define PDS_FWCTL_RPC_OPCODE_VER_MASK	GENMASK(23, PDS_FWCTL_RPC_OPCODE_VER_SHIFT)
> +
> +#define PDS_FWCTL_RPC_OPCODE_GET_CMD(op)	\
> +	(((op) & PDS_FWCTL_RPC_OPCODE_CMD_MASK) >> PDS_FWCTL_RPC_OPCODE_CMD_SHIFT)

Isn't that FIELD_GET?

> +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;
> +};

Use __aligned_u64 in the uapi headers

Jason