LKML Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] net: fm10k: check size from dma region
@ 2020-07-03 18:20 Zekun Shen
  2020-07-04 16:05 ` Alexander Duyck
  0 siblings, 1 reply; 5+ messages in thread
From: Zekun Shen @ 2020-07-03 18:20 UTC (permalink / raw)
  Cc: Zekun Shen, Jeff Kirsher, David S. Miller, Jakub Kicinski,
	intel-wired-lan, netdev, linux-kernel

Size is read from a dma region as input from device. Add sanity
check of size before calling dma_sync_single_range_for_cpu
with it.

This would prevent DMA-API warning: device driver tries to sync DMA
memory it has not allocated.

Signed-off-by: Zekun Shen <bruceshenzk@gmail.com>
---
 drivers/net/ethernet/intel/fm10k/fm10k_main.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/ethernet/intel/fm10k/fm10k_main.c b/drivers/net/ethernet/intel/fm10k/fm10k_main.c
index 17738b0a9..e020b346b 100644
--- a/drivers/net/ethernet/intel/fm10k/fm10k_main.c
+++ b/drivers/net/ethernet/intel/fm10k/fm10k_main.c
@@ -304,6 +304,11 @@ static struct sk_buff *fm10k_fetch_rx_buffer(struct fm10k_ring *rx_ring,
 	struct fm10k_rx_buffer *rx_buffer;
 	struct page *page;
 
+	if (unlikely(size > PAGE_SIZE)) {
+		dev_err(rx_ring->dev, "size %d exceeds PAGE_SIZE\n", size);
+		return NULL;
+	}
+
 	rx_buffer = &rx_ring->rx_buffer[rx_ring->next_to_clean];
 	page = rx_buffer->page;
 	prefetchw(page);
-- 
2.17.1


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] net: fm10k: check size from dma region
  2020-07-03 18:20 [PATCH] net: fm10k: check size from dma region Zekun Shen
@ 2020-07-04 16:05 ` Alexander Duyck
  2020-07-04 16:37   ` Zekun Shen
  0 siblings, 1 reply; 5+ messages in thread
From: Alexander Duyck @ 2020-07-04 16:05 UTC (permalink / raw)
  To: Zekun Shen
  Cc: Jeff Kirsher, David S. Miller, Jakub Kicinski, intel-wired-lan,
	Netdev, LKML

On Fri, Jul 3, 2020 at 11:21 AM Zekun Shen <bruceshenzk@gmail.com> wrote:
>
> Size is read from a dma region as input from device. Add sanity
> check of size before calling dma_sync_single_range_for_cpu
> with it.
>
> This would prevent DMA-API warning: device driver tries to sync DMA
> memory it has not allocated.
>
> Signed-off-by: Zekun Shen <bruceshenzk@gmail.com>
> ---
>  drivers/net/ethernet/intel/fm10k/fm10k_main.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/drivers/net/ethernet/intel/fm10k/fm10k_main.c b/drivers/net/ethernet/intel/fm10k/fm10k_main.c
> index 17738b0a9..e020b346b 100644
> --- a/drivers/net/ethernet/intel/fm10k/fm10k_main.c
> +++ b/drivers/net/ethernet/intel/fm10k/fm10k_main.c
> @@ -304,6 +304,11 @@ static struct sk_buff *fm10k_fetch_rx_buffer(struct fm10k_ring *rx_ring,
>         struct fm10k_rx_buffer *rx_buffer;
>         struct page *page;
>
> +       if (unlikely(size > PAGE_SIZE)) {
> +               dev_err(rx_ring->dev, "size %d exceeds PAGE_SIZE\n", size);
> +               return NULL;
> +       }
> +
>         rx_buffer = &rx_ring->rx_buffer[rx_ring->next_to_clean];
>         page = rx_buffer->page;
>         prefetchw(page);

The upper limitation for the size should be 2K or FM10K_RX_BUFSZ, not
PAGE_SIZE. Otherwise you are still capable of going out of bounds
because the offset is used within the page to push the start of the
region up by 2K.

If this is actually fixing the warning it makes me wonder if the code
performing the check is broken itself since we would still be
accessing outside of the accessible DMA range.

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] net: fm10k: check size from dma region
  2020-07-04 16:05 ` Alexander Duyck
@ 2020-07-04 16:37   ` Zekun Shen
  2020-07-04 19:41     ` Alexander Duyck
  0 siblings, 1 reply; 5+ messages in thread
From: Zekun Shen @ 2020-07-04 16:37 UTC (permalink / raw)
  To: Alexander Duyck
  Cc: Jeff Kirsher, David S. Miller, Jakub Kicinski, intel-wired-lan,
	Netdev, LKML

On Sat, Jul 04, 2020 at 09:05:48AM -0700, Alexander Duyck wrote:
> The upper limitation for the size should be 2K or FM10K_RX_BUFSZ, not
> PAGE_SIZE. Otherwise you are still capable of going out of bounds
> because the offset is used within the page to push the start of the
> region up by 2K.
PAGE_SIZE can drop the warning, as the dma allocated size is PAGE_SIZE.

> If this is actually fixing the warning it makes me wonder if the code
> performing the check is broken itself since we would still be
> accessing outside of the accessible DMA range.
The unbounded size is only passed to fm10k_add_rx_frag, which expects
and checks size to be less than FM10K_RX_HDR_LEN which is 256.

In this way, any boundary between 256 and 4K should work. I could address
that with a second version.

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] net: fm10k: check size from dma region
  2020-07-04 16:37   ` Zekun Shen
@ 2020-07-04 19:41     ` Alexander Duyck
  2020-07-04 22:11       ` Zekun Shen
  0 siblings, 1 reply; 5+ messages in thread
From: Alexander Duyck @ 2020-07-04 19:41 UTC (permalink / raw)
  To: Zekun Shen
  Cc: Jeff Kirsher, David S. Miller, Jakub Kicinski, intel-wired-lan,
	Netdev, LKML

On Sat, Jul 4, 2020 at 9:37 AM Zekun Shen <bruceshenzk@gmail.com> wrote:
>
> On Sat, Jul 04, 2020 at 09:05:48AM -0700, Alexander Duyck wrote:
> > The upper limitation for the size should be 2K or FM10K_RX_BUFSZ, not
> > PAGE_SIZE. Otherwise you are still capable of going out of bounds
> > because the offset is used within the page to push the start of the
> > region up by 2K.
> PAGE_SIZE can drop the warning, as the dma allocated size is PAGE_SIZE.

Yes, but the point I was getting at is that if you are just going to
squelch the warning, but leave the code broken then the warning isn't
of any use and might as well be discarded. Either you limit the value
to 2K which is what the hardware is expected to max out at anyway, or
you just skip the warning and assume hardware will do the right thing.
I'm not even sure this patch is worth the effort if it is just using
some dummy value that is still broken and simply squelches the
warning.

Could you provide more information about how you are encountering the
error? Is this something you are seeing with an actual fm10k device,
or is this something found via code review or static analysis?

> > If this is actually fixing the warning it makes me wonder if the code
> > performing the check is broken itself since we would still be
> > accessing outside of the accessible DMA range.
> The unbounded size is only passed to fm10k_add_rx_frag, which expects
> and checks size to be less than FM10K_RX_HDR_LEN which is 256.
>
> In this way, any boundary between 256 and 4K should work. I could address
> that with a second version.

I was referring to the code in the DMA-API that is generating the
warning being broken, not the code itself. If you can tell me how you
are getting to the warning it would be useful.

Anything over FM10K_RX_BUFSZ will break things. I think that is what
you are missing. The driver splits a single 4K page into 2 pieces and
then gives half off to the stack and uses the other half for the next
receive. If you have a value over 2K you are going to be overwritting
data in another buffer and/or attempting to access memory outside the
DMA region. Both of which would likely cause significant issues and
likely panic the system.

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] net: fm10k: check size from dma region
  2020-07-04 19:41     ` Alexander Duyck
@ 2020-07-04 22:11       ` Zekun Shen
  0 siblings, 0 replies; 5+ messages in thread
From: Zekun Shen @ 2020-07-04 22:11 UTC (permalink / raw)
  To: Alexander Duyck
  Cc: Jeff Kirsher, David S. Miller, Jakub Kicinski, intel-wired-lan,
	Netdev, LKML

On Sat, Jul 04, 2020 at 12:41:07PM -0700, Alexander Duyck wrote:
> On Sat, Jul 4, 2020 at 9:37 AM Zekun Shen <bruceshenzk@gmail.com> wrote:
> >
> > On Sat, Jul 04, 2020 at 09:05:48AM -0700, Alexander Duyck wrote:
> > > The upper limitation for the size should be 2K or FM10K_RX_BUFSZ, not
> > > PAGE_SIZE. Otherwise you are still capable of going out of bounds
> > > because the offset is used within the page to push the start of the
> > > region up by 2K.
> > PAGE_SIZE can drop the warning, as the dma allocated size is PAGE_SIZE.
>
> Yes, but the point I was getting at is that if you are just going to
> squelch the warning, but leave the code broken then the warning isn't
> of any use and might as well be discarded. Either you limit the value
> to 2K which is what the hardware is expected to max out at anyway, or
> you just skip the warning and assume hardware will do the right thing.
> I'm not even sure this patch is worth the effort if it is just using
> some dummy value that is still broken and simply squelches the
> warning.
>
> Could you provide more information about how you are encountering the
> error? Is this something you are seeing with an actual fm10k device,
> or is this something found via code review or static analysis?
I did not see it on a real device. I got the warning through emulation
and fuzzing, treating dma, mmio and interrupts as input vectors.
My research is on the peripheral/driver boundary.
>
> > > If this is actually fixing the warning it makes me wonder if the code
> > > performing the check is broken itself since we would still be
> > > accessing outside of the accessible DMA range.
> > The unbounded size is only passed to fm10k_add_rx_frag, which expects
> > and checks size to be less than FM10K_RX_HDR_LEN which is 256.
> >
> > In this way, any boundary between 256 and 4K should work. I could address
> > that with a second version.
>
> I was referring to the code in the DMA-API that is generating the
> warning being broken, not the code itself. If you can tell me how you
> are getting to the warning it would be useful.
>
> Anything over FM10K_RX_BUFSZ will break things. I think that is what
> you are missing. The driver splits a single 4K page into 2 pieces and
> then gives half off to the stack and uses the other half for the next
> receive. If you have a value over 2K you are going to be overwritting
> data in another buffer and/or attempting to access memory outside the
> DMA region. Both of which would likely cause significant issues and
> likely panic the system.
I agree. FM10K_RX_BUFSZ is the right boundary in that sense.

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-03 18:20 [PATCH] net: fm10k: check size from dma region Zekun Shen
2020-07-04 16:05 ` Alexander Duyck
2020-07-04 16:37   ` Zekun Shen
2020-07-04 19:41     ` Alexander Duyck
2020-07-04 22:11       ` Zekun Shen

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git
	git clone --mirror https://lore.kernel.org/lkml/8 lkml/git/8.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org
	public-inbox-index lkml

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git