[PATCH] intel_th: Fix a missing-check bug

[PATCH] intel_th: Fix a missing-check bug

[Wenwen Wang]

In msc_data_sz(), the 'valid_dw' field of the msc block descriptor 'bdesc'
is firstly checked to see whether the descriptor has a valid data width. If
yes, i.e., 'bdesc->valid_dw' is not 0, the data size of this descriptor
will be returned. It is worth noting that the data size is calculated from
'bdesc->valid_dw'. The problem here is that 'bdesc' actually points to a
consistent DMA region, which is allocated through dma_alloc_coherent() in
msc_buffer_win_alloc(). Given that the device also has the permission to
access this DMA region, it is possible that a malicious device controlled
by an attacker can modify the 'valid_dw' field after the if statement but
before the return statement in msc_data_sz(). Through this way, the device
can bypass the check and supply unexpected data width.

This patch copies the 'valid_dw' field to a local variable and then
performs the check and the calculation on the local variable to avoid the
above issue.

Signed-off-by: Wenwen Wang <wang6495@umn.edu>
---
 drivers/hwtracing/intel_th/msu.h | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/hwtracing/intel_th/msu.h b/drivers/hwtracing/intel_th/msu.h
index 9cc8ace..b7d846e 100644
--- a/drivers/hwtracing/intel_th/msu.h
+++ b/drivers/hwtracing/intel_th/msu.h
@@ -79,10 +79,12 @@ struct msc_block_desc {
 
 static inline unsigned long msc_data_sz(struct msc_block_desc *bdesc)
 {
-	if (!bdesc->valid_dw)
+	u32 valid_dw = bdesc->valid_dw;
+
+	if (!valid_dw)
 		return 0;
 
-	return bdesc->valid_dw * 4 - MSC_BDESC;
+	return valid_dw * 4 - MSC_BDESC;
 }
 
 static inline bool msc_block_wrapped(struct msc_block_desc *bdesc)
-- 
2.7.4

Re: [PATCH] intel_th: Fix a missing-check bug

[Wenwen Wang]

Hello,

Can anyone confirm this bug? Thanks!

Wenwen

On Fri, Oct 19, 2018 at 8:47 AM Wenwen Wang <wang6495@umn.edu> wrote:
>
> In msc_data_sz(), the 'valid_dw' field of the msc block descriptor 'bdesc'
> is firstly checked to see whether the descriptor has a valid data width. If
> yes, i.e., 'bdesc->valid_dw' is not 0, the data size of this descriptor
> will be returned. It is worth noting that the data size is calculated from
> 'bdesc->valid_dw'. The problem here is that 'bdesc' actually points to a
> consistent DMA region, which is allocated through dma_alloc_coherent() in
> msc_buffer_win_alloc(). Given that the device also has the permission to
> access this DMA region, it is possible that a malicious device controlled
> by an attacker can modify the 'valid_dw' field after the if statement but
> before the return statement in msc_data_sz(). Through this way, the device
> can bypass the check and supply unexpected data width.
>
> This patch copies the 'valid_dw' field to a local variable and then
> performs the check and the calculation on the local variable to avoid the
> above issue.
>
> Signed-off-by: Wenwen Wang <wang6495@umn.edu>
> ---
>  drivers/hwtracing/intel_th/msu.h | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/hwtracing/intel_th/msu.h b/drivers/hwtracing/intel_th/msu.h
> index 9cc8ace..b7d846e 100644
> --- a/drivers/hwtracing/intel_th/msu.h
> +++ b/drivers/hwtracing/intel_th/msu.h
> @@ -79,10 +79,12 @@ struct msc_block_desc {
>
>  static inline unsigned long msc_data_sz(struct msc_block_desc *bdesc)
>  {
> -       if (!bdesc->valid_dw)
> +       u32 valid_dw = bdesc->valid_dw;
> +
> +       if (!valid_dw)
>                 return 0;
>
> -       return bdesc->valid_dw * 4 - MSC_BDESC;
> +       return valid_dw * 4 - MSC_BDESC;
>  }
>
>  static inline bool msc_block_wrapped(struct msc_block_desc *bdesc)
> --
> 2.7.4
>

Re: [PATCH] intel_th: Fix a missing-check bug

[Alexander Shishkin]

Wenwen Wang <wang6495@umn.edu> writes:

> Hello,
>
> Can anyone confirm this bug? Thanks!

Commonly this burden lies with the author of the patch. If you fix a
bug, you need to be able to demonstrate it. If it's a mere hypothesis,
there needs to be a detailed analysis of how exactly can this be
exploited and what is the potential outcome of an exploit.

Some other questions that arise from looking at the patch, for example,
does gcc actually generate different code as a result of this patch?

> On Fri, Oct 19, 2018 at 8:47 AM Wenwen Wang <wang6495@umn.edu> wrote:
>>
>> In msc_data_sz(), the 'valid_dw' field of the msc block descriptor 'bdesc'
>> is firstly checked to see whether the descriptor has a valid data width. If
>> yes, i.e., 'bdesc->valid_dw' is not 0, the data size of this descriptor
>> will be returned. It is worth noting that the data size is calculated from
>> 'bdesc->valid_dw'. The problem here is that 'bdesc' actually points to a
>> consistent DMA region, which is allocated through dma_alloc_coherent() in
>> msc_buffer_win_alloc(). Given that the device also has the permission to
>> access this DMA region, it is possible that a malicious device controlled
>> by an attacker can modify the 'valid_dw' field after the if statement but
>> before the return statement in msc_data_sz(). Through this way, the
>> device

So, I *guess* you're assuming that an IOMMU will stop the malicious
device from overwriting the kernel code directly, so instead they're
reduced to breaking the driver's assumptions about the HW behavior, and
that is the reason why patches like this are sent in the first
place. This train of thought needs to be explained. Now, if we start
defending ourselves against malicious hardware, we need a comprehensive
analysis of possible attack vectors and their consequences. I'm not
convinced that it needs to be done in the first place, but if it does,
it sure needs to be better than grepping for a potential load after
validation.

>> can bypass the check and supply unexpected data width.
>>
>> This patch copies the 'valid_dw' field to a local variable and then
>> performs the check and the calculation on the local variable to avoid the
>> above issue.
>>
>> Signed-off-by: Wenwen Wang <wang6495@umn.edu>
>> ---
>>  drivers/hwtracing/intel_th/msu.h | 6 ++++--
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/hwtracing/intel_th/msu.h b/drivers/hwtracing/intel_th/msu.h
>> index 9cc8ace..b7d846e 100644
>> --- a/drivers/hwtracing/intel_th/msu.h
>> +++ b/drivers/hwtracing/intel_th/msu.h
>> @@ -79,10 +79,12 @@ struct msc_block_desc {
>>
>>  static inline unsigned long msc_data_sz(struct msc_block_desc *bdesc)
>>  {
>> -       if (!bdesc->valid_dw)
>> +       u32 valid_dw = bdesc->valid_dw;
>> +
>> +       if (!valid_dw)
>>                 return 0;
>>
>> -       return bdesc->valid_dw * 4 - MSC_BDESC;
>> +       return valid_dw * 4 - MSC_BDESC;

Or, the "malicious device" could just set valid_dw to something within
[1; MSC_BDESC/4), pass the check anyway and yield a more interesting
result, which may lead to an out-of-bounds access in the buffer reading
function. In other words, there's potential for improvement around this
function, but this patch misses it.

Regards,
--
Alex