Re: [PATCH] media: usb: ttusb-dec: avoid buffer overflow in ttusb_dec_handle_irq() when DMA failures/attacks occur - Jia-Ju Bai

From: Jia-Ju Bai <baijiaju1990@gmail.com>
To: Greg KH <gregkh@linuxfoundation.org>,
	Jia-Ju Bai <baijiaju1990@gmail.com>
Cc: mchehab@kernel.org, kstewart@linuxfoundation.org,
	tomasbortoli@gmail.com, sean@mess.org, allison@lohutok.net,
	tglx@linutronix.de, linux-media@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH] media: usb: ttusb-dec: avoid buffer overflow in ttusb_dec_handle_irq() when DMA failures/attacks occur
Date: Wed, 6 May 2020 18:13:01 +0800	[thread overview]
Message-ID: <0e4a86ee-8c4e-4ac3-8499-4e9a6ed7bd1e@gmail.com> (raw)
In-Reply-To: <20200505181042.GD1199718@kroah.com>

Hi Greg,

Thanks for the reply :)

On 2020/5/6 2:10, Greg KH wrote:
> On Tue, May 05, 2020 at 10:21:10PM +0800, Jia-Ju Bai wrote:
>> In this case, "buffer[4] - 1 < ARRAY_SIZE(rc_keys)"
>> can be first satisfied, and then the value of buffer[4] can be changed
>> to a large number, causing a buffer-overflow vulnerability.
> Um, maybe.  I agree testing and then using the value can cause problems
> when userspace provides you with that data and control, but for
> DMA-backed memory, we are in so much other trouble if that is the case.
>
>> To avoid the risk of this vulnerability, buffer[4] is assigned to a
>> non-DMA local variable "index" at the beginning of
>> ttusb_dec_handle_irq(), and then this variable replaces each use of
>> buffer[4] in the function.
> I strongly doubt this is even possible.  Can you describe how you can
> modify DMA memory and if so, would you do something tiny like this?
>

I have never modified DMA memory in the real world, but an attacker can 
use a malicious device to do this.
There is a video that shows how to use the Inception tool to perform DMA 
attacks and login in the Windows OS without password:
https://www.youtube.com/watch?v=HDhpy7RpUjM

Besides, the Windows OS resists against DMA attacks by disabling DMA of 
external devices by default:
http://support.microsoft.com/kb/2516445

Considering that this patch is for a USB media driver, I think that an 
attacker can just insert a malicious USB device to modify DMA memory and 
trigger this bug.
Besides, not related to this patch, some drivers use DMA to send/receive 
data (such as the URB used in USB drivers and ring descriptors used in 
network drivers). In this case, if the data is malicious and used as an 
array index through DMA, security problems may occur.

In my opinion, similar to the data from userspace, the data from 
hardware may be also malicious and should be checked.

Maybe we could discuss this issue with DMA driver developers?

Best wishes,
Jia-Ju Bai