[PATCH] niu: Add "overloaded" struct page union member

[PATCH] niu: Add "overloaded" struct page union member

[Kees Cook]

The randstruct GCC plugin gets upset when it sees struct addresspace
(which is randomized) being assigned to a struct page (which is not
randomized):

drivers/net/ethernet/sun/niu.c: In function 'niu_rx_pkt_ignore':
drivers/net/ethernet/sun/niu.c:3385:31: note: randstruct: casting between randomized structure pointer types (ssa): 'struct page' and 'struct address_space'

 3385 |                         *link = (struct page *) page->mapping;
      |                         ~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

It looks like niu.c is looking for an in-line place to chain its
allocated pages together and is overloading the "mapping" member, as it
is unused.

I expect this change will be met with alarm, given the strange corner
case it is. I wonder if, instead of "mapping", niu.c should instead be
using the "private" member? It wasn't clear to me if this was safe, and
I have no hardware to test with.

No meaningful machine code changes result after this change, and source
readability is improved.

Drop the randstruct exception now that there is no "confusing" cross-type
assignment.

Cc: "Matthew Wilcox (Oracle)" <willy@infradead.org>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Paolo Abeni <pabeni@redhat.com>
Cc: Du Cheng <ducheng2@gmail.com>
Cc: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
Cc: Vlastimil Babka <vbabka@suse.cz>
Cc: William Kucharski <william.kucharski@oracle.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Nathan Chancellor <nathan@kernel.org>
Cc: netdev@vger.kernel.org
Cc: linux-hardening@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 drivers/net/ethernet/sun/niu.c                | 17 ++++++++---------
 include/linux/mm_types.h                      |  7 +++++--
 scripts/gcc-plugins/randomize_layout_plugin.c |  2 --
 3 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/net/ethernet/sun/niu.c b/drivers/net/ethernet/sun/niu.c
index 42460c0885fc..75f0a1ce955b 100644
--- a/drivers/net/ethernet/sun/niu.c
+++ b/drivers/net/ethernet/sun/niu.c
@@ -3300,7 +3300,7 @@ static void niu_hash_page(struct rx_ring_info *rp, struct page *page, u64 base)
 	unsigned int h = niu_hash_rxaddr(rp, base);
 
 	page->index = base;
-	page->mapping = (struct address_space *) rp->rxhash[h];
+	page->overloaded = rp->rxhash[h];
 	rp->rxhash[h] = page;
 }
 
@@ -3382,11 +3382,11 @@ static int niu_rx_pkt_ignore(struct niu *np, struct rx_ring_info *rp)
 		rcr_size = rp->rbr_sizes[(val & RCR_ENTRY_PKTBUFSZ) >>
 					 RCR_ENTRY_PKTBUFSZ_SHIFT];
 		if ((page->index + PAGE_SIZE) - rcr_size == addr) {
-			*link = (struct page *) page->mapping;
+			*link = page->overloaded;
 			np->ops->unmap_page(np->device, page->index,
 					    PAGE_SIZE, DMA_FROM_DEVICE);
 			page->index = 0;
-			page->mapping = NULL;
+			page->overloaded = NULL;
 			__free_page(page);
 			rp->rbr_refill_pending++;
 		}
@@ -3451,11 +3451,11 @@ static int niu_process_rx_pkt(struct napi_struct *napi, struct niu *np,
 
 		niu_rx_skb_append(skb, page, off, append_size, rcr_size);
 		if ((page->index + rp->rbr_block_size) - rcr_size == addr) {
-			*link = (struct page *) page->mapping;
+			*link = page->overloaded;
 			np->ops->unmap_page(np->device, page->index,
 					    PAGE_SIZE, DMA_FROM_DEVICE);
 			page->index = 0;
-			page->mapping = NULL;
+			page->overloaded = NULL;
 			rp->rbr_refill_pending++;
 		} else
 			get_page(page);
@@ -3518,13 +3518,13 @@ static void niu_rbr_free(struct niu *np, struct rx_ring_info *rp)
 
 		page = rp->rxhash[i];
 		while (page) {
-			struct page *next = (struct page *) page->mapping;
+			struct page *next = page->overloaded;
 			u64 base = page->index;
 
 			np->ops->unmap_page(np->device, base, PAGE_SIZE,
 					    DMA_FROM_DEVICE);
 			page->index = 0;
-			page->mapping = NULL;
+			page->overloaded = NULL;
 
 			__free_page(page);
 
@@ -6440,8 +6440,7 @@ static void niu_reset_buffers(struct niu *np)
 
 				page = rp->rxhash[j];
 				while (page) {
-					struct page *next =
-						(struct page *) page->mapping;
+					struct page *next = page->overloaded;
 					u64 base = page->index;
 					base = base >> RBR_DESCR_ADDR_SHIFT;
 					rp->rbr[k++] = cpu_to_le32(base);
diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
index 8834e38c06a4..1cd5a1a93916 100644
--- a/include/linux/mm_types.h
+++ b/include/linux/mm_types.h
@@ -95,8 +95,11 @@ struct page {
 					unsigned int mlock_count;
 				};
 			};
-			/* See page-flags.h for PAGE_MAPPING_FLAGS */
-			struct address_space *mapping;
+			union {
+				/* See page-flags.h for PAGE_MAPPING_FLAGS */
+				struct address_space *mapping;
+				void *overloaded;
+			};
 			pgoff_t index;		/* Our offset within mapping. */
 			/**
 			 * @private: Mapping-private opaque data.
diff --git a/scripts/gcc-plugins/randomize_layout_plugin.c b/scripts/gcc-plugins/randomize_layout_plugin.c
index 727512eebb3b..38a8cf90f611 100644
--- a/scripts/gcc-plugins/randomize_layout_plugin.c
+++ b/scripts/gcc-plugins/randomize_layout_plugin.c
@@ -46,8 +46,6 @@ struct whitelist_entry {
 };
 
 static const struct whitelist_entry whitelist[] = {
-	/* NIU overloads mapping with page struct */
-	{ "drivers/net/ethernet/sun/niu.c", "page", "address_space" },
 	/* unix_skb_parms via UNIXCB() buffer */
 	{ "net/unix/af_unix.c", "unix_skb_parms", "char" },
 	{ }
-- 
2.32.0

Re: [PATCH] niu: Add "overloaded" struct page union member

[Christoph Hellwig]

On Mon, May 09, 2022 at 03:23:33PM -0700, Kees Cook wrote:
> The randstruct GCC plugin gets upset when it sees struct addresspace
> (which is randomized) being assigned to a struct page (which is not
> randomized):

Well, the right fix here is to remove this abuse from the driver, not
to legitimize it as part of a "driver" patch touching a core mm header
that doesn't even cc the mm list.

Re: [PATCH] niu: Add "overloaded" struct page union member

[Kees Cook]

On Tue, May 10, 2022 at 12:27:53AM -0700, Christoph Hellwig wrote:
> On Mon, May 09, 2022 at 03:23:33PM -0700, Kees Cook wrote:
> > The randstruct GCC plugin gets upset when it sees struct addresspace
> > (which is randomized) being assigned to a struct page (which is not
> > randomized):
> 
> Well, the right fix here is to remove this abuse from the driver, not
> to legitimize it as part of a "driver" patch touching a core mm header

Right, I didn't expect anyone to like the new "overloaded" member.
Mainly I'd just like to understand how niu _should_ be fixed. Is using
the "private" member the correct thing here?

> that doesn't even cc the mm list.

Oops, yes, sorry.

-- 
Kees Cook

Re: [PATCH] niu: Add "overloaded" struct page union member

[Matthew Wilcox]

On Tue, May 10, 2022 at 08:50:47AM -0700, Kees Cook wrote:
> On Tue, May 10, 2022 at 12:27:53AM -0700, Christoph Hellwig wrote:
> > On Mon, May 09, 2022 at 03:23:33PM -0700, Kees Cook wrote:
> > > The randstruct GCC plugin gets upset when it sees struct addresspace
> > > (which is randomized) being assigned to a struct page (which is not
> > > randomized):
> > 
> > Well, the right fix here is to remove this abuse from the driver, not
> > to legitimize it as part of a "driver" patch touching a core mm header
> 
> Right, I didn't expect anyone to like the new "overloaded" member.
> Mainly I'd just like to understand how niu _should_ be fixed. Is using
> the "private" member the correct thing here?

Well ... no.  We're not entirely set up yet to go to the good answer
that means we don't have to touch this driver again, and yet we're also
in a situation where we'll need to touch this driver at some point in
order to get rid of the way it abuses struct page before we can get to
our good place.

The eventual good answer is that we declare a driver-private memdesc
variant that has a ->link, ->base ->refcount and ->pfn (maybe it has more
than that; I'd have to really understand this driver to be completely
certain about what it needs).  Or perhaps there's a better way to handle
driver-allocated memory for this kind of networking card that this driver
should be converted to use.

I haven't looked into this case deeply enough to have strong thoughts
about how we should handle it, both now and in the glorious future.

Re: [PATCH] niu: Add "overloaded" struct page union member

[Kees Cook]

On Tue, May 10, 2022 at 06:27:41PM +0100, Matthew Wilcox wrote:
> On Tue, May 10, 2022 at 08:50:47AM -0700, Kees Cook wrote:
> > On Tue, May 10, 2022 at 12:27:53AM -0700, Christoph Hellwig wrote:
> > > On Mon, May 09, 2022 at 03:23:33PM -0700, Kees Cook wrote:
> > > > The randstruct GCC plugin gets upset when it sees struct addresspace
> > > > (which is randomized) being assigned to a struct page (which is not
> > > > randomized):
> > > 
> > > Well, the right fix here is to remove this abuse from the driver, not
> > > to legitimize it as part of a "driver" patch touching a core mm header
> > 
> > Right, I didn't expect anyone to like the new "overloaded" member.
> > Mainly I'd just like to understand how niu _should_ be fixed. Is using
> > the "private" member the correct thing here?
> 
> Well ... no.  We're not entirely set up yet to go to the good answer
> that means we don't have to touch this driver again, and yet we're also
> in a situation where we'll need to touch this driver at some point in
> order to get rid of the way it abuses struct page before we can get to
> our good place.
> 
> The eventual good answer is that we declare a driver-private memdesc
> variant that has a ->link, ->base ->refcount and ->pfn (maybe it has more
> than that; I'd have to really understand this driver to be completely
> certain about what it needs).  Or perhaps there's a better way to handle
> driver-allocated memory for this kind of networking card that this driver
> should be converted to use.
> 
> I haven't looked into this case deeply enough to have strong thoughts
> about how we should handle it, both now and in the glorious future.

Okay, in the meantime, I'll just add a casting wrapper with a big
comment to explain what I understand about it with some pointers back to
this and prior threads. :)

Thanks!

-- 
Kees Cook