linux-kernel - Re: [PATCH v2 1/2] 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] [day] [month] [year] [list]

Message-Id: <c9eca3df-d1ba-46f8-95c2-2da0b8c25803@vates.tech>
Date: Mon, 02 Dec 2024 17:21:46 +0000
From: "Thierry Escande" <thierry.escande@...es.tech>
To: Jürgen Groß <jgross@...e.com>, linux-kernel@...r.kernel.org, iommu@...ts.linux.dev
Cc: "Stefano Stabellini" <sstabellini@...nel.org>, "Oleksandr Tyshchenko" <oleksandr_tyshchenko@...m.com>, xen-devel@...ts.xenproject.org, jbeulich@...e.com
Subject: Re: [PATCH v2 1/2] xen/swiotlb: add alignment check for dma buffers


On 02/12/2024 09:27, Jürgen Groß wrote:
> On 29.11.24 18:36, Thierry Escande wrote:
>> Hi,
>>
>> On 16/09/2024 08:47, Juergen Gross wrote:
>>> diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
>>> index 35155258a7e2..ddf5b1df632e 100644
>>> --- 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);
>>> +    phys_addr_t algn = 1ULL << (get_order(size) + PAGE_SHIFT);
>>>         next_bfn = pfn_to_bfn(xen_pfn);
>>>   +    /* If buffer is physically aligned, ensure DMA alignment. */
>>> +    if (IS_ALIGNED(p, algn) &&
>>> +        !IS_ALIGNED(next_bfn << XEN_PAGE_SHIFT, algn))
>>> +        return 1;
>>
>> There is a regression in the mpt3sas driver because of this change.
>> When, in a dom0, this driver creates its DMA pool at probe time and
>> calls dma_pool_zalloc() (see [1]), the call to
>> range_straddles_page_boundary() (from xen_swiotlb_alloc_coherent())
>> returns 1 because of the alignment checks added by this patch. Then the
>> call to xen_create_contiguous_region() in xen_swiotlb_alloc_coherent()
>> fails because the passed size order is too big (> MAX_CONTIG_ORDER).
>> This driver sets the pool allocation block size to 2.3+ MBytes.
>>
>>  From previous discussions on the v1 patch, these checks are not
>> necessary from xen_swiotlb_alloc_coherent() that already ensures
>> alignment, right?
> 
> It ensures alignment regarding guest physical memory, but it doesn't do
> so for machine memory.
> 
> For DMA machine memory proper alignment might be needed, too, so the
> check is required. As an example, some crypto drivers seem to rely on
> proper machine memory alignment, which was the reason for those checks
> to be added.
> 
> Possible solutions include:
> 
> - rising the MAX_CONTIG_ORDER limit (to which value?)

Looks like the quick and easy solution. Bumping MAX_CONTIG_ORDER to 10
would allow 4MB pools, enough for this particular driver. I'll send a
patch if that sounds reasonable.

Regards,
Thierry

> - adding a way to allocate large DMA buffers with relaxed alignment
>   requirements (this will impact the whole DMA infrastructure plus
>   drivers like mp3sas which would need to use the new interface)
> - modify the mpt3sas driver to stay within the current limits
> - only guarantee proper machine memory alignment up to MAX_CONTIG_ORDER
>   (risking to introduce hard to diagnose bugs for the rare use cases of
>   such large buffers)
> 
> 
> Juergen