Message-ID: <CALs4sv0Pc0dhYvBU_PPawyN=c=fEvn+nq1uvpzjnKGn0BJjc4Q@mail.gmail.com>
Date: Fri, 6 Feb 2026 06:54:37 +0530
From: Pavan Chebbi <pavan.chebbi@...adcom.com>
To: Jason Gunthorpe <jgg@...pe.ca>
Cc: Andy Gospodarek <andrew.gospodarek@...adcom.com>, Saeed Mahameed <saeedm@...dia.com>, 
	michael.chan@...adcom.com, linux-kernel@...r.kernel.org, dave.jiang@...el.com, 
	Jonathan.Cameron@...wei.com, gospo@...adcom.com, selvin.xavier@...adcom.com, 
	leon@...nel.org, kalesh-anakkur.purayil@...adcom.com
Subject: Re: [PATCH v3 fwctl 4/5] fwctl/bnxt_fwctl: Add bnxt fwctl device

On Fri, Feb 6, 2026 at 6:09 AM Jason Gunthorpe <jgg@...pe.ca> wrote:
>
> On Thu, Feb 05, 2026 at 07:19:43PM -0500, Andy Gospodarek wrote:
> > On Thu, Feb 5, 2026 at 1:42 PM Jason Gunthorpe <jgg@...pe.ca> wrote:
> > >
> > > On Thu, Feb 05, 2026 at 12:47:53PM -0500, Andy Gospodarek wrote:
> > >
> > > >    Jason, this is all done in bnxtctl_fw_rpc() and the functions it calls
> > > >    in this (or the v4 version) of this patch.
> > >
> > > Oh.. No.. You can't do this:
> > >
> > > +       rpc_in.msg = memdup_user(u64_to_user_ptr(msg->req), msg->req_len);
> > >          // ^^ eventually becomes fw_msg
> > > +               dma_ptr = fw_msg->msg + msg->offset;
> > > +               *(__le64 *)(dma_ptr) = cpu_to_le64(dma_addr[i]);
> > >
> > > There is nothing in here which ensures that every DMA physical address
> > > in the mailbox given from userspace is *actually forced to a value by
> > > the kernel*
> > >
> > > The kernel only overwrites values from the user controlled struct
> > > fwctl_dma_info_bnxt array.
> > >
> > > Meaning userspace can just specify any physical address in the
> > > message, not include any fwctl_dma_info_bnxt list and it will happily
> > > send the user controlled physical address to the FW. Thus userspace
> > > can DMA to whatever memory it likes.
> > >
> > > IOW this is a *HUGE* security error!
> > >
> >
> > I'll let Pavan chime in when he sees this, but I'm not sure that's
> > correct.  You might be right that we have a problem that is as bad as
> > you describe, but it's also possible we could do a better job
> > explaining how these DMA operations work and why they are needed.
>
> I think it is pretty clear, the DMA addreses are being read by FW from
> the same memory buffer that the userspace fully controls.
>
> Look at the code, the design takes an offset list from userspace and
> patches in DMA addresses from the kernel into the same buffer that
> the userspace data was copied from.
>
> NOTHING is checking that the offset list is security acceptable for
> the command!!
>
> > As I read through these again, I realize that what might have made
> > these easier to read would be simply changing the names of a few of
> > these structures when they are really just buffers being copied
> > between user and kernel space -- not buffers that are actually being
> > DMAed from hardware to userspace.
>
> I understand how the buffer copying works on the happy path, I am
> pointing out that if the user submits a command that has
>
> +struct fwctl_rpc_bnxt {
> +       __aligned_u64 req;
> +       __u32 req_len;
> +       __u32 timeout;
> +       __u32 num_dma;
>           ^^^^^^^^^^^
>
> === 0
>
> then the kernel doesn't even allocate any of these DMA buffers, it
> doesn't change the command buffer and it sends raw userspace data
> directly to FW.
>
> Since that very same command buffer obviously contains the DMA
> addresses to use there is nothing blocking userspace from doing this.
>
> Stated another way, the kernel must NOT take in
>
> +struct fwctl_dma_info_bnxt {
> +       __aligned_u64 data;
> +       __u32 len;
> +       __u16 offset;
>             ^^^^^^^
>
> This parameter from userspace! The kernel MUST know exactly where all
> DMA addresses lie in the message now and into the future to guarentee
> that it and only it writes a DMA address of the kernel controlled
> DMA bounce buffer.
>
> Allowing a userspace controlled DMA address to pass into the device
> directly from userspace is a complete security fail.

I think its important to quickly clarify the mechanism lest the whole
thing adds more questions.
Let me explain with an example as to how its happening. I am sure its
the case of naming the struct as *dma* info that is freaking everyone
out.
When a user has no additional host buffers to add to a FW command, it
just sets the num_dma = 0. At that time, req member of struct
fwctl_rpc_bnxt has the entire user buffer to carry FW command i/p and
o/p.

Sometimes a FW command needs a separate additional host buffers beyond
the input message. The input message will carry the address at which
additional host buffers are found.
Example is a command like below where FW will return additional data
in a separate tx_stat_host_addr and rx_stat_host_addr

struct hwrm_port_qstats_input {
        __le16 req_type;
        __le16 cmpl_ring;
        __le16 seq_id;
        __le16 target_id;
        __le64 resp_addr;
        __le16 port_id;
        u8 flags;
        u8 unused_0[5];
        __le64 tx_stat_host_addr;
       __le64 rx_stat_host_addr;
};

Now the user will tell driver that this FW command has two additional
host buffers that driver needs to supply to FW. So it will say num_dma
= 2 in the rpc message. And describe each additional buffer so that
driver can allocate dma_alloc_coherent memory for it.
FW command expects the DMA-able buffers' address be present at
'tx_stat_host_addr' and 'rx_stat_host_addr' fields of
hwrm_port_qstats_input. Here is where 'offset' comes into the picture.
In the above example, user is only telling the driver, offset of
'__le64 tx_stat_host_addr' in the input structure, using the 'offset'
field of the additional buffer description.

Does that make the mechanism clear...

>
> Jason

Download attachment "smime.p7s" of type "application/pkcs7-signature" (5469 bytes)

Powered by blists - more mailing lists