linux-kernel - Re: [PATCH] xen/swiotlb: add alignment check for dma buffers

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <alpine.DEB.2.22.394.2409131728420.1417852@ubuntu-linux-20-04-desktop>
Date: Fri, 13 Sep 2024 17:38:28 -0700 (PDT)
From: Stefano Stabellini <sstabellini@...nel.org>
To: Jan Beulich <jbeulich@...e.com>
cc: Juergen Gross <jgross@...e.com>, 
    Stefano Stabellini <sstabellini@...nel.org>, 
    Oleksandr Tyshchenko <oleksandr_tyshchenko@...m.com>, 
    xen-devel@...ts.xenproject.org, linux-kernel@...r.kernel.org, 
    iommu@...ts.linux.dev
Subject: Re: [PATCH] xen/swiotlb: add alignment check for dma buffers

On Fri, 13 Sep 2024, Jan Beulich wrote:
> On 13.09.2024 16:56, Juergen Gross wrote:
> > --- a/drivers/xen/swiotlb-xen.c
> > +++ b/drivers/xen/swiotlb-xen.c
> > @@ -78,9 +78,15 @@ static inline int range_straddles_page_boundary(phys_addr_t p, size_t size)
> >  {
> >  	unsigned long next_bfn, xen_pfn = XEN_PFN_DOWN(p);
> >  	unsigned int i, nr_pages = XEN_PFN_UP(xen_offset_in_page(p) + size);
> > +	unsigned int order = get_order(size);
> >  
> >  	next_bfn = pfn_to_bfn(xen_pfn);
> >  
> > +	/* If buffer is physically aligned, ensure DMA alignment. */
> > +	if (IS_ALIGNED(p, 1UL << (order + PAGE_SHIFT)) &&
> 
> Why this check? xen_swiotlb_alloc_coherent() guarantees it, while
> xen_swiotlb_free_coherent() only checks properties of the original
> allocation. And for xen_swiotlb_map_page() this looks actively
> wrong to me, in case that function was called with offset non-zero.

I understand xen_swiotlb_alloc_coherent and xen_swiotlb_free_coherent
not needing the check, but I think we might need the check for
xen_swiotlb_map_page. At that point, I would keep the check for all
callers. Unless there is another way to detect whether the mapping needs
alignment specifically for map_page?

For the offset, in theory if the device needs alignment, the offset
should be zero? If the offset is not zero, then there should be no
alignment requirement. The way Juergen wrote the check, we would take
the fast path if offset != zero, which makes sense to me.