* [PATCH] dma-debug: Fix overflow issue in bucket_find_contain
@ 2022-07-30 11:41 ` yf.wang
0 siblings, 0 replies; 6+ messages in thread
From: yf.wang @ 2022-07-30 11:41 UTC (permalink / raw)
To: Christoph Hellwig, Robin Murphy, Marek Szyprowski,
Matthias Brugger, open list:DMA MAPPING HELPERS, open list,
moderated list:ARM/Mediatek SoC support,
moderated list:ARM/Mediatek SoC support
Cc: wsd_upstream, Libo Kang, Ning Li, Yong Wu, Miles Chen,
Yunfei Wang, jianjiao zeng
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;
+
/*
* 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);
}
--
2.18.0
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH] dma-debug: Fix overflow issue in bucket_find_contain
@ 2022-07-30 11:41 ` yf.wang
0 siblings, 0 replies; 6+ messages in thread
From: yf.wang @ 2022-07-30 11:41 UTC (permalink / raw)
To: Christoph Hellwig, Robin Murphy, Marek Szyprowski,
Matthias Brugger, open list:DMA MAPPING HELPERS, open list,
moderated list:ARM/Mediatek SoC support,
moderated list:ARM/Mediatek SoC support
Cc: wsd_upstream, Libo Kang, Ning Li, Yong Wu, Miles Chen,
Yunfei Wang, jianjiao zeng
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;
+
/*
* 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);
}
--
2.18.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] dma-debug: Fix overflow issue in bucket_find_contain
2022-07-30 11:41 ` yf.wang
@ 2022-08-01 17:57 ` Christoph Hellwig
-1 siblings, 0 replies; 6+ messages in thread
From: Christoph Hellwig @ 2022-08-01 17:57 UTC (permalink / raw)
To: yf.wang
Cc: Christoph Hellwig, Robin Murphy, Marek Szyprowski,
Matthias Brugger, open list:DMA MAPPING HELPERS, open list,
moderated list:ARM/Mediatek SoC support,
moderated list:ARM/Mediatek SoC support, wsd_upstream, Libo Kang,
Ning Li, Yong Wu, Miles Chen, jianjiao zeng
> + unsigned int shift = (1 << HASH_FN_SHIFT);
Please just add a constant for that right nxt to the HASH_FN_SHIFT
definition.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] dma-debug: Fix overflow issue in bucket_find_contain
@ 2022-08-01 17:57 ` Christoph Hellwig
0 siblings, 0 replies; 6+ messages in thread
From: Christoph Hellwig @ 2022-08-01 17:57 UTC (permalink / raw)
To: yf.wang
Cc: Christoph Hellwig, Robin Murphy, Marek Szyprowski,
Matthias Brugger, open list:DMA MAPPING HELPERS, open list,
moderated list:ARM/Mediatek SoC support,
moderated list:ARM/Mediatek SoC support, wsd_upstream, Libo Kang,
Ning Li, Yong Wu, Miles Chen, jianjiao zeng
> + unsigned int shift = (1 << HASH_FN_SHIFT);
Please just add a constant for that right nxt to the HASH_FN_SHIFT
definition.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] dma-debug: Fix overflow issue in bucket_find_contain
2022-07-30 11:41 ` yf.wang
@ 2022-08-01 18:47 ` Robin Murphy
-1 siblings, 0 replies; 6+ messages in thread
From: Robin Murphy @ 2022-08-01 18:47 UTC (permalink / raw)
To: yf.wang, Christoph Hellwig, Marek Szyprowski, Matthias Brugger,
open list:DMA MAPPING HELPERS, open list,
moderated list:ARM/Mediatek SoC support,
moderated list:ARM/Mediatek SoC support
Cc: wsd_upstream, Libo Kang, Ning Li, Yong Wu, Miles Chen, jianjiao zeng
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);
> }
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] dma-debug: Fix overflow issue in bucket_find_contain
@ 2022-08-01 18:47 ` Robin Murphy
0 siblings, 0 replies; 6+ messages in thread
From: Robin Murphy @ 2022-08-01 18:47 UTC (permalink / raw)
To: yf.wang, Christoph Hellwig, Marek Szyprowski, Matthias Brugger,
open list:DMA MAPPING HELPERS, open list,
moderated list:ARM/Mediatek SoC support,
moderated list:ARM/Mediatek SoC support
Cc: wsd_upstream, Libo Kang, Ning Li, Yong Wu, Miles Chen, jianjiao zeng
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);
> }
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-08-01 18:49 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-07-30 11:41 [PATCH] dma-debug: Fix overflow issue in bucket_find_contain yf.wang
2022-07-30 11:41 ` yf.wang
2022-08-01 17:57 ` Christoph Hellwig
2022-08-01 17:57 ` Christoph Hellwig
2022-08-01 18:47 ` Robin Murphy
2022-08-01 18:47 ` Robin Murphy
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.