Message-ID: <20260206003939.GA943673@ziepe.ca>
Date: Thu, 5 Feb 2026 20:39:39 -0400
From: Jason Gunthorpe <jgg@...pe.ca>
To: Andy Gospodarek <andrew.gospodarek@...adcom.com>
Cc: Pavan Chebbi <pavan.chebbi@...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 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.

Jason

Powered by blists - more mailing lists