Re: [PATCH] dma-debug: Fix overflow issue in bucket_find_contain

[Robin Murphy]

On 2022-07-30 12:41, yf.wang@xxxxxxxxxxxx wrote:
> From: Yunfei Wang <yf.wang@xxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxx>
> Signed-off-by: Yunfei Wang <yf.wang@xxxxxxxxxxxx>
> ---
>   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);
>   	}
>