[PATCH] arch/arm/mm/dma-mapping.c: fix __alloc_from_pool returning a dirty buffer

[PATCH] arch/arm/mm/dma-mapping.c: fix __alloc_from_pool returning a dirty buffer

[Igor Klochko]

Hi, 

A small patch for __alloc_from_pool to clean the buffer before returning.
 
---
 arch/arm/mm/dma-mapping.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
index c135f6e37a00..bb2bb3ab497a 100644
--- a/arch/arm/mm/dma-mapping.c
+++ b/arch/arm/mm/dma-mapping.c
@@ -366,6 +366,7 @@ static void *__alloc_from_pool(size_t size, struct page **ret_page)
 
 		*ret_page = phys_to_page(phys);
 		ptr = (void *)val;
+		memset(ptr, 0, size);
 	}
 
 	return ptr;
-- 
2.39.0


Kind regards,
Igor Klochko

Re: [PATCH] arch/arm/mm/dma-mapping.c: fix __alloc_from_pool returning a dirty buffer

[Christoph Hellwig]

On Fri, Dec 23, 2022 at 03:15:47PM +0100, Igor Klochko wrote:
> 
> Hi, 
> 
> A small patch for __alloc_from_pool to clean the buffer before returning.

This does look correct.  The "normal" allocators seems to do the
memset through __dma_clear_buffer, but the __alloc_from_pool seems to be
missing it.  Please write a proper changelog with a signoff, and
preferably a Fixes tag if you can find what introduced this.  Also
the ARM code needs to go to the ARM maintainer and arm mailing list.

Re: [PATCH] arch/arm/mm/dma-mapping.c: fix __alloc_from_pool returning a dirty buffer

[Igor Klochko]

Thanks Christoph,

Added fixes and a changelog.
This issue is present in all current LTS versions.

----
Buffers allocated by __alloc_from_pool() should be zeroed out as done by other allocators.
Certain drivers expect a clean buffer and clearing the buffer is beneficial from the security point of view.
---
Fixes: 36d0fd2198da3 (*arm: use genalloc for the atomic pool*)
Signed-off-by: Igor Klochko <igor.klochko@gmail.com>
---
 arch/arm/mm/dma-mapping.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
index c135f6e37a00..bb2bb3ab497a 100644
--- a/arch/arm/mm/dma-mapping.c
+++ b/arch/arm/mm/dma-mapping.c
@@ -366,6 +366,7 @@ static void *__alloc_from_pool(size_t size, struct page **ret_page)
 
 		*ret_page = phys_to_page(phys);
 		ptr = (void *)val;
+		memset(ptr, 0, size);
 	}
 
 	return ptr;
-- 
2.39.0




Kind regards,
Igor Klochko




On 23/12/2022 15:39, Christoph Hellwig wrote:
> On Fri, Dec 23, 2022 at 03:15:47PM +0100, Igor Klochko wrote:
>>
>> Hi, 
>>
>> A small patch for __alloc_from_pool to clean the buffer before returning.
> 
> This does look correct.  The "normal" allocators seems to do the
> memset through __dma_clear_buffer, but the __alloc_from_pool seems to be
> missing it.  Please write a proper changelog with a signoff, and
> preferably a Fixes tag if you can find what introduced this.  Also
> the ARM code needs to go to the ARM maintainer and arm mailing list.

Re: [PATCH] arch/arm/mm/dma-mapping.c: fix __alloc_from_pool returning a dirty buffer

[Igor Klochko]

Thanks Christoph,

Added fixes and a changelog.
This issue is present in all current LTS versions.

----
Buffers allocated by __alloc_from_pool() should be zeroed out as done by other allocators.
Certain drivers expect a clean buffer and clearing the buffer is beneficial from the security point of view.
---
Fixes: 36d0fd2198da3 (*arm: use genalloc for the atomic pool*)
Signed-off-by: Igor Klochko <igor.klochko@gmail.com>
---
 arch/arm/mm/dma-mapping.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
index c135f6e37a00..bb2bb3ab497a 100644
--- a/arch/arm/mm/dma-mapping.c
+++ b/arch/arm/mm/dma-mapping.c
@@ -366,6 +366,7 @@ static void *__alloc_from_pool(size_t size, struct page **ret_page)
 
 		*ret_page = phys_to_page(phys);
 		ptr = (void *)val;
+		memset(ptr, 0, size);
 	}
 
 	return ptr;
-- 
2.39.0




Kind regards,
Igor Klochko




On 23/12/2022 15:39, Christoph Hellwig wrote:
> On Fri, Dec 23, 2022 at 03:15:47PM +0100, Igor Klochko wrote:
>>
>> Hi, 
>>
>> A small patch for __alloc_from_pool to clean the buffer before returning.
> 
> This does look correct.  The "normal" allocators seems to do the
> memset through __dma_clear_buffer, but the __alloc_from_pool seems to be
> missing it.  Please write a proper changelog with a signoff, and
> preferably a Fixes tag if you can find what introduced this.  Also
> the ARM code needs to go to the ARM maintainer and arm mailing list.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arch/arm/mm/dma-mapping.c: fix __alloc_from_pool returning a dirty buffer

[Russell King (Oracle)]

On Fri, Dec 23, 2022 at 11:51:43PM +0100, Igor Klochko wrote:
> Thanks Christoph,
> 
> Added fixes and a changelog.
> This issue is present in all current LTS versions.
> 
> ----
> Buffers allocated by __alloc_from_pool() should be zeroed out as done by other allocators.
> Certain drivers expect a clean buffer and clearing the buffer is beneficial from the security point of view.
> ---
> Fixes: 36d0fd2198da3 (*arm: use genalloc for the atomic pool*)
> Signed-off-by: Igor Klochko <igor.klochko@gmail.com>
> ---
>  arch/arm/mm/dma-mapping.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
> index c135f6e37a00..bb2bb3ab497a 100644
> --- a/arch/arm/mm/dma-mapping.c
> +++ b/arch/arm/mm/dma-mapping.c
> @@ -366,6 +366,7 @@ static void *__alloc_from_pool(size_t size, struct page **ret_page)
>  
>  		*ret_page = phys_to_page(phys);
>  		ptr = (void *)val;
> +		memset(ptr, 0, size);

I'm not up to speed with the current structure of the DMA implementation
on ARM, but isn't this going to make cache lines dirty for the returned
buffer, which will corrupt DMA on non-coherent devices?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH] arch/arm/mm/dma-mapping.c: fix __alloc_from_pool returning a dirty buffer

[Russell King (Oracle)]

On Fri, Dec 23, 2022 at 11:51:43PM +0100, Igor Klochko wrote:
> Thanks Christoph,
> 
> Added fixes and a changelog.
> This issue is present in all current LTS versions.
> 
> ----
> Buffers allocated by __alloc_from_pool() should be zeroed out as done by other allocators.
> Certain drivers expect a clean buffer and clearing the buffer is beneficial from the security point of view.
> ---
> Fixes: 36d0fd2198da3 (*arm: use genalloc for the atomic pool*)
> Signed-off-by: Igor Klochko <igor.klochko@gmail.com>
> ---
>  arch/arm/mm/dma-mapping.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
> index c135f6e37a00..bb2bb3ab497a 100644
> --- a/arch/arm/mm/dma-mapping.c
> +++ b/arch/arm/mm/dma-mapping.c
> @@ -366,6 +366,7 @@ static void *__alloc_from_pool(size_t size, struct page **ret_page)
>  
>  		*ret_page = phys_to_page(phys);
>  		ptr = (void *)val;
> +		memset(ptr, 0, size);

I'm not up to speed with the current structure of the DMA implementation
on ARM, but isn't this going to make cache lines dirty for the returned
buffer, which will corrupt DMA on non-coherent devices?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arch/arm/mm/dma-mapping.c: fix __alloc_from_pool returning a dirty buffer

[Robin Murphy]

On 03/01/2023 9:51 am, Russell King (Oracle) wrote:
> On Fri, Dec 23, 2022 at 11:51:43PM +0100, Igor Klochko wrote:
>> Thanks Christoph,
>>
>> Added fixes and a changelog.
>> This issue is present in all current LTS versions.
>>
>> ----
>> Buffers allocated by __alloc_from_pool() should be zeroed out as done by other allocators.
>> Certain drivers expect a clean buffer and clearing the buffer is beneficial from the security point of view.
>> ---
>> Fixes: 36d0fd2198da3 (*arm: use genalloc for the atomic pool*)
>> Signed-off-by: Igor Klochko <igor.klochko@gmail.com>
>> ---
>>   arch/arm/mm/dma-mapping.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
>> index c135f6e37a00..bb2bb3ab497a 100644
>> --- a/arch/arm/mm/dma-mapping.c
>> +++ b/arch/arm/mm/dma-mapping.c
>> @@ -366,6 +366,7 @@ static void *__alloc_from_pool(size_t size, struct page **ret_page)
>>   
>>   		*ret_page = phys_to_page(phys);
>>   		ptr = (void *)val;
>> +		memset(ptr, 0, size);
> 
> I'm not up to speed with the current structure of the DMA implementation
> on ARM, but isn't this going to make cache lines dirty for the returned
> buffer, which will corrupt DMA on non-coherent devices?

The virtual address of the pool, and thus VAs within the pool as 
returned by gen_pool_alloc(), should be a non-cacheable remap set up by 
atomic_pool_init(), so it looks appropriate to me. In fact "ptr" here 
appears to be the final return value propagated all the way back out to 
the caller of dma_alloc_coherent(), so if that were dodgy then there 
would already be problems afoot.

Robin.

Re: [PATCH] arch/arm/mm/dma-mapping.c: fix __alloc_from_pool returning a dirty buffer

[Robin Murphy]

On 03/01/2023 9:51 am, Russell King (Oracle) wrote:
> On Fri, Dec 23, 2022 at 11:51:43PM +0100, Igor Klochko wrote:
>> Thanks Christoph,
>>
>> Added fixes and a changelog.
>> This issue is present in all current LTS versions.
>>
>> ----
>> Buffers allocated by __alloc_from_pool() should be zeroed out as done by other allocators.
>> Certain drivers expect a clean buffer and clearing the buffer is beneficial from the security point of view.
>> ---
>> Fixes: 36d0fd2198da3 (*arm: use genalloc for the atomic pool*)
>> Signed-off-by: Igor Klochko <igor.klochko@gmail.com>
>> ---
>>   arch/arm/mm/dma-mapping.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
>> index c135f6e37a00..bb2bb3ab497a 100644
>> --- a/arch/arm/mm/dma-mapping.c
>> +++ b/arch/arm/mm/dma-mapping.c
>> @@ -366,6 +366,7 @@ static void *__alloc_from_pool(size_t size, struct page **ret_page)
>>   
>>   		*ret_page = phys_to_page(phys);
>>   		ptr = (void *)val;
>> +		memset(ptr, 0, size);
> 
> I'm not up to speed with the current structure of the DMA implementation
> on ARM, but isn't this going to make cache lines dirty for the returned
> buffer, which will corrupt DMA on non-coherent devices?

The virtual address of the pool, and thus VAs within the pool as 
returned by gen_pool_alloc(), should be a non-cacheable remap set up by 
atomic_pool_init(), so it looks appropriate to me. In fact "ptr" here 
appears to be the final return value propagated all the way back out to 
the caller of dma_alloc_coherent(), so if that were dodgy then there 
would already be problems afoot.

Robin.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel