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

[Greg KH]

On Tue, May 05, 2020 at 10:21:10PM +0800, Jia-Ju Bai wrote:
> In ttusb_dec_init_usb():
>   dec->irq_buffer = usb_alloc_coherent(...)

Nice.

> Thus, "dec->irq_buffer" is a DMA value, and it is assigned to "buffer"
> in ttusb_dec_handle_irq():
>   char *buffer = dec->irq_buffer;

Nice.

> When DMA failures or attacks occur, the value of buffer[4] can be
> changed at any time.

Wait, how can that happen?

The kernel can not protect itself from something like that in each
individual driver, that's impossible.  Your patch just makes that
"window" a few cycles smaller than before.

> 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?

> 
> Signed-off-by: Jia-Ju Bai <baijiaju1990@xxxxxxxxx>
> ---
>  drivers/media/usb/ttusb-dec/ttusb_dec.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/usb/ttusb-dec/ttusb_dec.c b/drivers/media/usb/ttusb-dec/ttusb_dec.c
> index 3198f9624b7c..8543c552515b 100644
> --- a/drivers/media/usb/ttusb-dec/ttusb_dec.c
> +++ b/drivers/media/usb/ttusb-dec/ttusb_dec.c
> @@ -250,6 +250,7 @@ static void ttusb_dec_handle_irq( struct urb *urb)
>  	struct ttusb_dec *dec = urb->context;
>  	char *buffer = dec->irq_buffer;
>  	int retval;
> +	u8 index = buffer[4];
>  
>  	switch(urb->status) {
>  		case 0: /*success*/
> @@ -281,11 +282,11 @@ static void ttusb_dec_handle_irq( struct urb *urb)
>  		 * this should/could be added later ...
>  		 * for now lets report each signal as a key down and up
>  		 */
> -		if (buffer[4] - 1 < ARRAY_SIZE(rc_keys)) {
> -			dprintk("%s:rc signal:%d\n", __func__, buffer[4]);
> -			input_report_key(dec->rc_input_dev, rc_keys[buffer[4] - 1], 1);
> +		if (index - 1 < ARRAY_SIZE(rc_keys)) {
> +			dprintk("%s:rc signal:%d\n", __func__, index);
> +			input_report_key(dec->rc_input_dev, rc_keys[index - 1], 1);
>  			input_sync(dec->rc_input_dev);
> -			input_report_key(dec->rc_input_dev, rc_keys[buffer[4] - 1], 0);
> +			input_report_key(dec->rc_input_dev, rc_keys[index - 1], 0);
>  			input_sync(dec->rc_input_dev);
>  		}
>  	}

The above complaints aside, the patch does make sense for the simple
fact that it might reduce the number of memory accesses.

But, the compiler might already be doing this type of optimization
anyway, right?  So your original issue might not even be a problem.

Anyhow, the patch looks fine, but it's a minor cleanup, not a fix for
any sort of bug/security issue at all.  If you change the text in the
changelog area, I'll be glad to ack it.

thanks,

greg k-h