[RFC 0/1] swiotlb: Mitigate potential DoS caused by BUG_ON()

[RFC 0/1] swiotlb: Mitigate potential DoS caused by BUG_ON()

[Brian Johannesmeyer]

We identified a security issue in the swiotlb unmapping operation, stemming
from the way some drivers save streaming DMA addresses. This issue can
potentially be exploited by a malicious peripheral device to cause a
denial-of-service (DoS) via a kernel panic.

**Disclosure Context**
We previously reported a similar issue to the Linux kernel security team.
However, they advised submitting such cases directly to the relevant
subsystem maintainers and the hardening mailing list, because Linux
implicitly assumes hardware can be trusted.

**Threat Model**
While Linux drivers typically trust their hardware, there may be specific
drivers that do not operate under this assumption. Hence, we consider a
malicious peripheral device that corrupts DMA data to exploit the kernel.
In this scenario, the device manipulates driver-initialized data (similar
to the attack described in the Thunderclap paper [0]) to achieve a DoS.

**Background**
Streaming DMA is often used as follows:
(1) A driver maps a buffer to DMA using dma_map_single(), which returns a
DMA address. This address is then saved by the driver for later use.
(2) When the buffer is no longer needed, the driver unmaps it using
dma_unmap_single(), passing the previously saved DMA address.
(3) Under certain conditions---such as the driver using direct mapping
operations, and the mapped buffer requiring a swiotlb
buffer---dma_unmap_single() calls swiotlb_release_slots(). Here, the saved
DMA address is passed as its tlb_addr argument.

**Vulnerability**
It is common for drivers to store the DMA address returned by
dma_map_single() into a coherent DMA region, which can be manipulated by a
malicious device. For example, the E100 driver and RealTek 8139C+ driver
map socket buffers into streaming DMA and save their DMA addresses to
coherent DMA data. While these drivers might assume trusted hardware, this
behavior is not necessarily unique to them.

If an untrusted device corrupts the DMA address, it can influence the
tlb_addr argument passed to swiotlb_release_slots(). Inside this function,
tlb_addr is used to calculate aindex, which is validated via BUG_ON(aindex
>= mem->nareas). By manipulating the DMA address, an attacker can trigger
this assertion, resulting in a kernel panic and DoS.

I have a PDF document that illustrates how these steps work. Please let me
know if you would like me to share it with you.

**Proposed mitigation**
To address this issue, two potential approaches are possible:

(1) Mitigating the *initialization* of attacker data: Prevent drivers from
saving critical data, such as DMA addresses, in attacker-controllable
regions.
(2) Mitigating the *use* of attacker data: Modify the handling of critical
data, such as in the BUG_ON() statement, to not result in catastrophic
outcomes like kernel panics.

While approach (1) is more complete, it is more challenging to deploy
universally. Hence, we propose mitigating this issue with approach (2):
i.e., replacing the BUG_ON() assertion with more graceful error handling.
The attached patch implements this change, ensuring the kernel can handle
the condition safely without triggering a panic.

**Request for Feedback**
I am not deeply familiar with the swiotlb internals, so I would appreciate
any feedback on the patch. In particular, is there a more graceful way to
handle the error than returning?

Thanks,

Brian Johannesmeyer

[0] Link: https://www.csl.sri.com/~neumann/ndss-iommu.pdf

Brian Johannesmeyer (1):
  swiotlb: Replace BUG_ON() with graceful error handling

 kernel/dma/swiotlb.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

-- 
2.34.1

[RFC 1/1] swiotlb: Replace BUG_ON() with graceful error handling

[Brian Johannesmeyer]

Replace the BUG_ON() assertion in swiotlb_release_slots() with a
conditional check and return. This change prevents a corrupted tlb_addr
from causing a kernel panic.

Co-developed-by: Raphael Isemann <teemperor@gmail.com>
Signed-off-by: Raphael Isemann <teemperor@gmail.com>
Signed-off-by: Brian Johannesmeyer <bjohannesmeyer@gmail.com>
---
 kernel/dma/swiotlb.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
index aa0a4a220719..54b4f9665772 100644
--- a/kernel/dma/swiotlb.c
+++ b/kernel/dma/swiotlb.c
@@ -834,7 +834,11 @@ static void swiotlb_release_slots(struct device *dev, phys_addr_t tlb_addr)
 	 * While returning the entries to the free list, we merge the entries
 	 * with slots below and above the pool being returned.
 	 */
-	BUG_ON(aindex >= mem->nareas);
+	if (unlikely(aindex >= mem->nareas)) {
+		dev_err(dev, "%s: invalid area index (%d >= %d)\n", __func__,
+			aindex, mem->nareas);
+		return;
+	}
 
 	spin_lock_irqsave(&area->lock, flags);
 	if (index + nslots < ALIGN(index + 1, IO_TLB_SEGSIZE))
-- 
2.34.1

Re: [RFC 0/1] swiotlb: Mitigate potential DoS caused by BUG_ON()

[Brian Johannesmeyer]

On Fri, Nov 22, 2024 at 12:13 PM Brian Johannesmeyer
<bjohannesmeyer@gmail.com> wrote:
>
> We identified a security issue in the swiotlb unmapping operation, stemming
> from the way some drivers save streaming DMA addresses. This issue can
> potentially be exploited by a malicious peripheral device to cause a
> denial-of-service (DoS) via a kernel panic.
>
> **Disclosure Context**
> We previously reported a similar issue to the Linux kernel security team.
> However, they advised submitting such cases directly to the relevant
> subsystem maintainers and the hardening mailing list, because Linux
> implicitly assumes hardware can be trusted.
>
> **Threat Model**
> While Linux drivers typically trust their hardware, there may be specific
> drivers that do not operate under this assumption. Hence, we consider a
> malicious peripheral device that corrupts DMA data to exploit the kernel.
> In this scenario, the device manipulates driver-initialized data (similar
> to the attack described in the Thunderclap paper [0]) to achieve a DoS.
>
> **Background**
> Streaming DMA is often used as follows:
> (1) A driver maps a buffer to DMA using dma_map_single(), which returns a
> DMA address. This address is then saved by the driver for later use.
> (2) When the buffer is no longer needed, the driver unmaps it using
> dma_unmap_single(), passing the previously saved DMA address.
> (3) Under certain conditions---such as the driver using direct mapping
> operations, and the mapped buffer requiring a swiotlb
> buffer---dma_unmap_single() calls swiotlb_release_slots(). Here, the saved
> DMA address is passed as its tlb_addr argument.
>
> **Vulnerability**
> It is common for drivers to store the DMA address returned by
> dma_map_single() into a coherent DMA region, which can be manipulated by a
> malicious device. For example, the E100 driver and RealTek 8139C+ driver
> map socket buffers into streaming DMA and save their DMA addresses to
> coherent DMA data. While these drivers might assume trusted hardware, this
> behavior is not necessarily unique to them.
>
> If an untrusted device corrupts the DMA address, it can influence the
> tlb_addr argument passed to swiotlb_release_slots(). Inside this function,
> tlb_addr is used to calculate aindex, which is validated via BUG_ON(aindex
> >= mem->nareas). By manipulating the DMA address, an attacker can trigger
> this assertion, resulting in a kernel panic and DoS.
>
> I have a PDF document that illustrates how these steps work. Please let me
> know if you would like me to share it with you.
>
> **Proposed mitigation**
> To address this issue, two potential approaches are possible:
>
> (1) Mitigating the *initialization* of attacker data: Prevent drivers from
> saving critical data, such as DMA addresses, in attacker-controllable
> regions.
> (2) Mitigating the *use* of attacker data: Modify the handling of critical
> data, such as in the BUG_ON() statement, to not result in catastrophic
> outcomes like kernel panics.
>
> While approach (1) is more complete, it is more challenging to deploy
> universally. Hence, we propose mitigating this issue with approach (2):
> i.e., replacing the BUG_ON() assertion with more graceful error handling.
> The attached patch implements this change, ensuring the kernel can handle
> the condition safely without triggering a panic.
>
> **Request for Feedback**
> I am not deeply familiar with the swiotlb internals, so I would appreciate
> any feedback on the patch. In particular, is there a more graceful way to
> handle the error than returning?
>
> Thanks,
>
> Brian Johannesmeyer
>
> [0] Link: https://www.csl.sri.com/~neumann/ndss-iommu.pdf
>
> Brian Johannesmeyer (1):
>   swiotlb: Replace BUG_ON() with graceful error handling
>
>  kernel/dma/swiotlb.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> --
> 2.34.1
>

Whoops -- didn't send to the hardening mailing list. Adding it now.

-Brian

Re: [RFC 1/1] swiotlb: Replace BUG_ON() with graceful error handling

[Brian Johannesmeyer]

On Fri, Nov 22, 2024 at 12:13 PM Brian Johannesmeyer
<bjohannesmeyer@gmail.com> wrote:
>
> Replace the BUG_ON() assertion in swiotlb_release_slots() with a
> conditional check and return. This change prevents a corrupted tlb_addr
> from causing a kernel panic.
>
> Co-developed-by: Raphael Isemann <teemperor@gmail.com>
> Signed-off-by: Raphael Isemann <teemperor@gmail.com>
> Signed-off-by: Brian Johannesmeyer <bjohannesmeyer@gmail.com>
> ---
>  kernel/dma/swiotlb.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
> index aa0a4a220719..54b4f9665772 100644
> --- a/kernel/dma/swiotlb.c
> +++ b/kernel/dma/swiotlb.c
> @@ -834,7 +834,11 @@ static void swiotlb_release_slots(struct device *dev, phys_addr_t tlb_addr)
>          * While returning the entries to the free list, we merge the entries
>          * with slots below and above the pool being returned.
>          */
> -       BUG_ON(aindex >= mem->nareas);
> +       if (unlikely(aindex >= mem->nareas)) {
> +               dev_err(dev, "%s: invalid area index (%d >= %d)\n", __func__,
> +                       aindex, mem->nareas);
> +               return;
> +       }
>
>         spin_lock_irqsave(&area->lock, flags);
>         if (index + nslots < ALIGN(index + 1, IO_TLB_SEGSIZE))
> --
> 2.34.1
>

Whoops -- didn't send to the hardening mailing list. Adding it now.

-Brian

Re: [RFC 1/1] swiotlb: Replace BUG_ON() with graceful error handling

[Christoph Hellwig]

On Fri, Nov 22, 2024 at 08:13:04PM +0100, Brian Johannesmeyer wrote:
> Replace the BUG_ON() assertion in swiotlb_release_slots() with a
> conditional check and return. This change prevents a corrupted tlb_addr
> from causing a kernel panic.

We'll at least want a WARN_ON_ONCE here.  But what is the threat model
here?  The tlb_addr is handed out by swiotlb and should not be modified
by the driver.

Re: [RFC 0/1] swiotlb: Mitigate potential DoS caused by BUG_ON()

[Christoph Hellwig]

On Fri, Nov 22, 2024 at 08:13:03PM +0100, Brian Johannesmeyer wrote:
> It is common for drivers to store the DMA address returned by
> dma_map_single() into a coherent DMA region, which can be manipulated by a
> malicious device. For example, the E100 driver and RealTek 8139C+ driver
> map socket buffers into streaming DMA and save their DMA addresses to
> coherent DMA data. While these drivers might assume trusted hardware, this
> behavior is not necessarily unique to them.

FYI, while I don't mind replacing the BUG_ON with a WARN_ON and leaking
the swiotlb allocation, the part where the addresses are mapped so that
the device can modify them is probably what need to be fixed, and it
would also be useful to have documentation in the tree discouraging it.

Re: [RFC 0/1] swiotlb: Mitigate potential DoS caused by BUG_ON()

[Robin Murphy]

On 2024-11-22 8:33 pm, Brian Johannesmeyer wrote:
> On Fri, Nov 22, 2024 at 12:13 PM Brian Johannesmeyer
> <bjohannesmeyer@gmail.com> wrote:
>>
>> We identified a security issue in the swiotlb unmapping operation, stemming
>> from the way some drivers save streaming DMA addresses. This issue can
>> potentially be exploited by a malicious peripheral device to cause a
>> denial-of-service (DoS) via a kernel panic.
>>
>> **Disclosure Context**
>> We previously reported a similar issue to the Linux kernel security team.
>> However, they advised submitting such cases directly to the relevant
>> subsystem maintainers and the hardening mailing list, because Linux
>> implicitly assumes hardware can be trusted.
>>
>> **Threat Model**
>> While Linux drivers typically trust their hardware, there may be specific
>> drivers that do not operate under this assumption. Hence, we consider a
>> malicious peripheral device that corrupts DMA data to exploit the kernel.
>> In this scenario, the device manipulates driver-initialized data (similar
>> to the attack described in the Thunderclap paper [0]) to achieve a DoS.
>>
>> **Background**
>> Streaming DMA is often used as follows:
>> (1) A driver maps a buffer to DMA using dma_map_single(), which returns a
>> DMA address. This address is then saved by the driver for later use.
>> (2) When the buffer is no longer needed, the driver unmaps it using
>> dma_unmap_single(), passing the previously saved DMA address.
>> (3) Under certain conditions---such as the driver using direct mapping
>> operations, and the mapped buffer requiring a swiotlb
>> buffer---dma_unmap_single() calls swiotlb_release_slots(). Here, the saved
>> DMA address is passed as its tlb_addr argument.

This is hardly a SWIOTLB problem. If a driver allows its device to 
corrupt what is effectively driver-internal data, then the device can 
still easily crash a non-coherent system without SWIOTLB, by writing a 
DMA address for which phys_to_virt() returns a bogus unmapped VA which 
causes a cache maintenance instruction to fault. Or potentially do far 
worse by redirecting a DMA_FROM_DEVICE cache invalidation to some other 
valid VA to destroy some recently-written data.

Even with an IOMMU, whilst it should be relatively contained, it would 
still be able to mess with the IOMMU pagetables by causing unmapping of 
wrong IOVAs, and spam up the kernel log with warnings and driver errors 
(more than it already could) as the iommu-dma state gets progressively 
more out-of-sync with itself over repeated shenanigans.

I also have no issue with changing a BUG_ON() to a WARN() where the 
former is gratuitously unnecessary anyway - a common precedent in the 
SWIOTLB code, it seems - but let's not pretend we're meaningfully 
mitigating anything here. If a driver has made this naive assumption 
about DMA addresses, who knows how many other ways it could also be 
confused by a malicious device writing unexepected values to descriptor 
fields?

Thanks,
Robin.

>> **Vulnerability**
>> It is common for drivers to store the DMA address returned by
>> dma_map_single() into a coherent DMA region, which can be manipulated by a
>> malicious device. For example, the E100 driver and RealTek 8139C+ driver
>> map socket buffers into streaming DMA and save their DMA addresses to
>> coherent DMA data. While these drivers might assume trusted hardware, this
>> behavior is not necessarily unique to them.
>>
>> If an untrusted device corrupts the DMA address, it can influence the
>> tlb_addr argument passed to swiotlb_release_slots(). Inside this function,
>> tlb_addr is used to calculate aindex, which is validated via BUG_ON(aindex
>>> = mem->nareas). By manipulating the DMA address, an attacker can trigger
>> this assertion, resulting in a kernel panic and DoS.
>>
>> I have a PDF document that illustrates how these steps work. Please let me
>> know if you would like me to share it with you.
>>
>> **Proposed mitigation**
>> To address this issue, two potential approaches are possible:
>>
>> (1) Mitigating the *initialization* of attacker data: Prevent drivers from
>> saving critical data, such as DMA addresses, in attacker-controllable
>> regions.
>> (2) Mitigating the *use* of attacker data: Modify the handling of critical
>> data, such as in the BUG_ON() statement, to not result in catastrophic
>> outcomes like kernel panics.
>>
>> While approach (1) is more complete, it is more challenging to deploy
>> universally. Hence, we propose mitigating this issue with approach (2):
>> i.e., replacing the BUG_ON() assertion with more graceful error handling.
>> The attached patch implements this change, ensuring the kernel can handle
>> the condition safely without triggering a panic.
>>
>> **Request for Feedback**
>> I am not deeply familiar with the swiotlb internals, so I would appreciate
>> any feedback on the patch. In particular, is there a more graceful way to
>> handle the error than returning?
>>
>> Thanks,
>>
>> Brian Johannesmeyer
>>
>> [0] Link: https://www.csl.sri.com/~neumann/ndss-iommu.pdf
>>
>> Brian Johannesmeyer (1):
>>    swiotlb: Replace BUG_ON() with graceful error handling
>>
>>   kernel/dma/swiotlb.c | 6 +++++-
>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> --
>> 2.34.1
>>
> 
> Whoops -- didn't send to the hardening mailing list. Adding it now.
> 
> -Brian

Re: [RFC 1/1] swiotlb: Replace BUG_ON() with graceful error handling

[Robin Murphy]

On 2024-11-22 7:13 pm, Brian Johannesmeyer wrote:
> Replace the BUG_ON() assertion in swiotlb_release_slots() with a
> conditional check and return. This change prevents a corrupted tlb_addr
> from causing a kernel panic.

Hmm, looking again, how exactly *does* this happen? To get here from 
swiotlb_unmap_single(), swiotlb_find_pool() has already determined that 
"tlb_addr" is within the range belonging to "mem", so if it is somehow 
possible for it to then convert into an out-of-bounds index, maybe that 
does actually imply some bug in SWIOTLB itself where "mem" is 
misconfigured...

Thanks,
Robin.

> Co-developed-by: Raphael Isemann <teemperor@gmail.com>
> Signed-off-by: Raphael Isemann <teemperor@gmail.com>
> Signed-off-by: Brian Johannesmeyer <bjohannesmeyer@gmail.com>
> ---
>   kernel/dma/swiotlb.c | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
> index aa0a4a220719..54b4f9665772 100644
> --- a/kernel/dma/swiotlb.c
> +++ b/kernel/dma/swiotlb.c
> @@ -834,7 +834,11 @@ static void swiotlb_release_slots(struct device *dev, phys_addr_t tlb_addr)
>   	 * While returning the entries to the free list, we merge the entries
>   	 * with slots below and above the pool being returned.
>   	 */
> -	BUG_ON(aindex >= mem->nareas);
> +	if (unlikely(aindex >= mem->nareas)) {
> +		dev_err(dev, "%s: invalid area index (%d >= %d)\n", __func__,
> +			aindex, mem->nareas);
> +		return;
> +	}
>   
>   	spin_lock_irqsave(&area->lock, flags);
>   	if (index + nslots < ALIGN(index + 1, IO_TLB_SEGSIZE))

Re: [RFC 1/1] swiotlb: Replace BUG_ON() with graceful error handling

[Brian Johannesmeyer]

Thank you for your feedback and your patience. Apologies for the
delayed response --- I've had some personal matters arise in the past
couple weeks.

I had initially prepared responses addressing each of your points, but
after revisiting Robin's insightful observation below, it seems this
issue has already been resolved in the latest kernel. While I’m still
happy to address any further questions or comments, it appears that
additional discussion may no longer be necessary.

On Mon, Nov 25, 2024 at 8:03 AM Robin Murphy <robin.murphy@arm.com> wrote:
> Hmm, looking again, how exactly *does* this happen? To get here from
> swiotlb_unmap_single(), swiotlb_find_pool() has already determined that
> "tlb_addr" is within the range belonging to "mem", so if it is somehow
> possible for it to then convert into an out-of-bounds index, maybe that
> does actually imply some bug in SWIOTLB itself where "mem" is
> misconfigured...

Great observation. Upon reviewing the latest kernel, I realized I had
analyzed an older version of the code that lacked commit [0]. This
commit introduces swiotlb_find_pool() into swiotlb_tbl_unmap_single(),
effectively mitigating the issue. Specifically, if addr is corrupted,
swiotlb_find_pool() would return NULL, causing
swiotlb_tbl_unmap_single() to now return without invoking
__swiotlb_tbl_unmap_single(). Consequently, the failed BUG_ON in
swiotlb_release_slots() is avoided. My apologies for overlooking this
recent change and not verifying the code path in the latest kernel.

That said, converting the BUG_ON to a WARN_ON_ONCE might still be a
useful improvement, even if the immediate issue is resolved.

Sincerely,

Brian Johannesmeyer

[0] commit 7296f2301a057493e97b07739213c6e864f76891 ("swiotlb: reduce
swiotlb pool lookups")