lkml.org 
[lkml]   [2022]   [Aug]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH] dma-debug: Fix overflow issue in bucket_find_contain
From
On 2022-07-30 12:41, yf.wang@mediatek.com wrote:
> From: Yunfei Wang <yf.wang@mediatek.com>
>
> There are two issue:
> 1. If max_rang is set to 0xFFFF_FFFF, and __hash_bucket_find always
> returns NULL, the rang will be accumulated. When rang is accumulated
> to 0xFFFF_E000, after executing rang += (1 << HASH_FN_SHIFT) again,
> rang will overflow to 0, making it impossible to exit the while loop.
> 2. dev_addr reduce maybe overflow.
>
> So, add range and dev_addr check to avoid overflow.
>
> Signed-off-by: jianjiao zeng <jianjiao.zeng@mediatek.com>
> Signed-off-by: Yunfei Wang <yf.wang@mediatek.com>
> ---
> kernel/dma/debug.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/dma/debug.c b/kernel/dma/debug.c
> index ad731f7858c9..9d7d54cd4c63 100644
> --- a/kernel/dma/debug.c
> +++ b/kernel/dma/debug.c
> @@ -352,6 +352,7 @@ static struct dma_debug_entry *bucket_find_contain(struct hash_bucket **bucket,
>
> unsigned int max_range = dma_get_max_seg_size(ref->dev);
> struct dma_debug_entry *entry, index = *ref;
> + unsigned int shift = (1 << HASH_FN_SHIFT);
> unsigned int range = 0;
>
> while (range <= max_range) {
> @@ -360,12 +361,15 @@ static struct dma_debug_entry *bucket_find_contain(struct hash_bucket **bucket,
> if (entry)
> return entry;
>
> + if (max_range - range < shift || index.dev_addr < shift)
> + return NULL;

This seems a bit clunky since the first condition here effectively makes
the loop condition redundant.

FWIW I found the whole "range" business here rather hard to make sense
of - personally I'd calculate a lower bound for the address then just
iterate down to that, but maybe that's just me :/

Otherwise, at the very least we should be capping max_range so that the
loop doesn't go beyond HASH_SIZE iterations and pointlessly search the
same buckets more than once - it's stupid to even *get* to the point of
having to worry about that overflowing. Whether we really care about
dev_addr underflow is then another matter.

Really it would seem even more logical to make this a lower-level
function that can walk round the dma_entry_hash array directly and not
have to monkey about with the fake "index" entry at all, but cleaning up
the almost-unnecessary amount of internal abstractions here is maybe
more work than it's worth at this point.

Robin.

> +
> /*
> * Nothing found, go back a hash bucket
> */
> put_hash_bucket(*bucket, *flags);
> - range += (1 << HASH_FN_SHIFT);
> - index.dev_addr -= (1 << HASH_FN_SHIFT);
> + range += shift;
> + index.dev_addr -= shift;
> *bucket = get_hash_bucket(&index, flags);
> }
>

\
 
 \ /
  Last update: 2022-08-01 20:49    [W:0.047 / U:0.032 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site