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: <5b3a256b-7dd1-48bd-ba11-4766180ba0e3@amd.com>
Date: Wed, 5 Mar 2025 18:02:22 -0800
From: "Nelson, Shannon" <shannon.nelson@....com>
To: Jason Gunthorpe <jgg@...dia.com>,
 Jonathan Cameron <Jonathan.Cameron@...wei.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, 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 9:24 AM, Jason Gunthorpe wrote:
> On Tue, Mar 04, 2025 at 05:08:08PM +0800, Jonathan Cameron wrote:
>>> +   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.
> 
> Yes, please remove or degrade to dbg all the prints that userspace
> could trigger.

Sure

> 
>>> +   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");
> 
> kzalloc is already super noisy if it fails

I know ... I get dinged on this all the time, but I still add these 
because there are multiple allocs in this one function and it isn't 
immediately obvious in the trace which alloc failed.  I could push each 
alloc off to separate functions to get that info in the trace, but it 
seems to me that's extra unnecessary code to get the same one more line.


> 
>>> +                   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;
> 
> etc
> 
>>> +   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);
> 
> Triggerable by a malformed RPC?

That or misbehaving firmware.

> 
>>> +           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");
> 
> Triggerable by a malformed user provided pointer

yes

> 
> Jason

Thanks,
sln