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] [day] [month] [year] [list]

Message-ID: <1ea48dc6-6c26-43aa-bdfc-a754d8590853@amd.com>
Date: Wed, 5 Mar 2025 18:07:37 -0800
From: "Nelson, Shannon" <shannon.nelson@....com>
To: Jason Gunthorpe <jgg@...dia.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 3/4/2025 11:52 AM, Jason Gunthorpe wrote:
> 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?

Hmm... I think so.  I'll poke at that.

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

Sure.

Thanks,
sln



> 
> Jason