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.
> > + 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
> > + 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?
> > + 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
Jason
