netdev - Re: [PATCH v10 1/2] net/bnx2x: Prevent access to a freed page in page

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <917ccece-42f3-40a7-b3c7-fc30a3f9bc84@linux.ibm.com>
Date: Wed, 6 Mar 2024 15:13:10 -0600
From: Thinh Tran <thinhtr@...ux.ibm.com>
To: jacob.e.keller@...el.com
Cc: VENKATA.SAI.DUGGI@....com, abdhalee@...ibm.com, davem@...emloft.net,
        drc@...ux.vnet.ibm.com, edumazet@...gle.com, kuba@...nel.org,
        manishc@...vell.com, netdev@...r.kernel.org, pabeni@...hat.com,
        simon.horman@...igine.com, skalluru@...vell.com,
        thinhtr@...ux.vnet.ibm.com
Subject: Re: [PATCH v10 1/2] net/bnx2x: Prevent access to a freed page in
 page_pool

Apologies for the delayed response. I did not receive this email and 
some others in my mailbox.


> Doesn't this still leave a race window where put_page was already called
> but page hasn't yet been set NULL? I think you either need to assign
> NULL first (and possibly WRITE_ONCE or a barrier depending on platform?)
> or some other serialization mechanism to ensure only one thread runs here?
 >
> I guess the issue you're seeing is that bnx2x_free_rx_sge_range calls
> bnx2x_free_rx_sge even if the page was already removed? Does that mean

yes

> you already have some other serialization ensuring that you can't have
> both threads call put_page simultaneously?

The callers to bnx2x_free_rx_sge_range() are under rtnl_lock(), which 
should handle the serialization.

The crash occurs in the bnx2x_free_rx_sge() function due to accessing a 
NULL pointer.

799  static inline void bnx2x_free_rx_sge(struct bnx2x *bp,
800				struct bnx2x_fastpath *fp, u16 index)
801  {
802	struct sw_rx_page *sw_buf = &fp->rx_page_ring[index];
803     struct page *page = sw_buf->page;
804	struct eth_rx_sge *sge = &fp->rx_sge_ring[index];
.....
810	/* Since many fragments can share the same page, make sure to
811	 * only unmap and free the page once.
812	 */
813	dma_unmap_page(&bp->pdev->dev, dma_unmap_addr(sw_buf, mapping),
814		       SGE_PAGE_SIZE, DMA_FROM_DEVICE);
815
816	put_page(page);
...
}

This happens because sw_buf was set to NULL after the call to 
dma_unmap_page(), called by the preceding thread.
The patch checking if that page in the pool is already freed, there is 
nothing else to do.

Thinh Tran