2021-02-07 23:12:06

by Colin King

Subject: [PATCH] drm/amdgpu: fix potential integer overflow on shift of a int

From: Colin Ian King <[email protected]>

The left shift of int 32 bit integer constant 1 is evaluated using 32
bit arithmetic and then assigned to an unsigned 64 bit integer. In the
case where *frag is 32 or more this can lead to an oveflow. Avoid this
by shifting 1ULL.

Addresses-Coverity: ("Unintentional integer overflow")
Fixes: dfcd99f6273e ("drm/amdgpu: meld together VM fragment and huge page handling")
Signed-off-by: Colin Ian King <[email protected]>
---
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 9d19078246c8..53a925600510 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -1412,7 +1412,7 @@ static void amdgpu_vm_fragment(struct amdgpu_vm_update_params *params,
*frag = max_frag;
*frag_end = end & ~((1ULL << max_frag) - 1);
} else {
- *frag_end = start + (1 << *frag);
+ *frag_end = start + (1ULL << *frag);
}
}

--
2.29.2

2021-02-08 09:37:37

by Christian König

Subject: Re: [PATCH] drm/amdgpu: fix potential integer overflow on shift of a int

Am 08.02.21 um 00:07 schrieb Colin King:
> From: Colin Ian King <[email protected]>
>
> The left shift of int 32 bit integer constant 1 is evaluated using 32
> bit arithmetic and then assigned to an unsigned 64 bit integer. In the
> case where *frag is 32 or more this can lead to an oveflow. Avoid this
> by shifting 1ULL.

Well that can't happen. Take a look at the code in that function:

>                 max_frag = 31;
...
>         if (*frag >= max_frag) {
>                 *frag = max_frag;
>                 *frag_end = end & ~((1ULL << max_frag) - 1);
>         } else {
>                 *frag_end = start + (1 << *frag);
>         }

But I'm fine with applying the patch if it silences your warning.

Regards,
Christian.

>
> Addresses-Coverity: ("Unintentional integer overflow")
> Fixes: dfcd99f6273e ("drm/amdgpu: meld together VM fragment and huge page handling")
> Signed-off-by: Colin Ian King <[email protected]>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 9d19078246c8..53a925600510 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -1412,7 +1412,7 @@ static void amdgpu_vm_fragment(struct amdgpu_vm_update_params *params,
> *frag = max_frag;
> *frag_end = end & ~((1ULL << max_frag) - 1);
> } else {
> - *frag_end = start + (1 << *frag);
> + *frag_end = start + (1ULL << *frag);
> }
> }
>

2021-02-08 11:33:48

by walter harms

Subject: AW: [PATCH] drm/amdgpu: fix potential integer overflow on shift of a int

i am curious:
what is the win to have a unsigned 64 bit integer in the first
place ?

re,
wh
________________________________________
Von: Christian K?nig <[email protected]>
Gesendet: Montag, 8. Februar 2021 10:17:42
An: Colin King; Alex Deucher; David Airlie; Daniel Vetter; Huang Rui; Junwei Zhang; [email protected]; [email protected]
Cc: [email protected]; [email protected]
Betreff: Re: [PATCH] drm/amdgpu: fix potential integer overflow on shift of a int

Am 08.02.21 um 00:07 schrieb Colin King:
> From: Colin Ian King <[email protected]>
>
> The left shift of int 32 bit integer constant 1 is evaluated using 32
> bit arithmetic and then assigned to an unsigned 64 bit integer. In the
> case where *frag is 32 or more this can lead to an oveflow. Avoid this
> by shifting 1ULL.

Well that can't happen. Take a look at the code in that function:

> max_frag = 31;
...
> if (*frag >= max_frag) {
> *frag = max_frag;
> *frag_end = end & ~((1ULL << max_frag) - 1);
> } else {
> *frag_end = start + (1 << *frag);
> }

But I'm fine with applying the patch if it silences your warning.

Regards,
Christian.

>
> Addresses-Coverity: ("Unintentional integer overflow")
> Fixes: dfcd99f6273e ("drm/amdgpu: meld together VM fragment and huge page handling")
> Signed-off-by: Colin Ian King <[email protected]>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 9d19078246c8..53a925600510 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -1412,7 +1412,7 @@ static void amdgpu_vm_fragment(struct amdgpu_vm_update_params *params,
> *frag = max_frag;
> *frag_end = end & ~((1ULL << max_frag) - 1);
> } else {
> - *frag_end = start + (1 << *frag);
> + *frag_end = start + (1ULL << *frag);
> }
> }
>

2021-02-08 12:34:25

by Christian König

Subject: Re: [PATCH] drm/amdgpu: fix potential integer overflow on shift of a int

For start and end?  The hardware has 48 bit address space and that won't
fit into 32bits.

Only the fragment handling can't do more than 2GB at the same time.

Christian.

Am 08.02.21 um 12:05 schrieb Walter Harms:
> i am curious:
> what is the win to have a unsigned 64 bit integer in the first
> place ?
>
> re,
> wh
> ________________________________________
> Von: Christian König <[email protected]>
> Gesendet: Montag, 8. Februar 2021 10:17:42
> An: Colin King; Alex Deucher; David Airlie; Daniel Vetter; Huang Rui; Junwei Zhang; [email protected]; [email protected]
> Cc: [email protected]; [email protected]
> Betreff: Re: [PATCH] drm/amdgpu: fix potential integer overflow on shift of a int
>
> Am 08.02.21 um 00:07 schrieb Colin King:
>> From: Colin Ian King <[email protected]>
>>
>> The left shift of int 32 bit integer constant 1 is evaluated using 32
>> bit arithmetic and then assigned to an unsigned 64 bit integer. In the
>> case where *frag is 32 or more this can lead to an oveflow. Avoid this
>> by shifting 1ULL.
> Well that can't happen. Take a look at the code in that function:
>
>> max_frag = 31;
> ...
>> if (*frag >= max_frag) {
>> *frag = max_frag;
>> *frag_end = end & ~((1ULL << max_frag) - 1);
>> } else {
>> *frag_end = start + (1 << *frag);
>> }
> But I'm fine with applying the patch if it silences your warning.
>
> Regards,
> Christian.
>
>> Addresses-Coverity: ("Unintentional integer overflow")
>> Fixes: dfcd99f6273e ("drm/amdgpu: meld together VM fragment and huge page handling")
>> Signed-off-by: Colin Ian King <[email protected]>
>> ---
>> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> index 9d19078246c8..53a925600510 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> @@ -1412,7 +1412,7 @@ static void amdgpu_vm_fragment(struct amdgpu_vm_update_params *params,
>> *frag = max_frag;
>> *frag_end = end & ~((1ULL << max_frag) - 1);
>> } else {
>> - *frag_end = start + (1 << *frag);
>> + *frag_end = start + (1ULL << *frag);
>> }
>> }
>>

2021-02-08 18:29:56

by walter harms

Subject: AW: [PATCH] drm/amdgpu: fix potential integer overflow on shift of a int

thx for info
________________________________________
Von: Christian K?nig <[email protected]>
Gesendet: Montag, 8. Februar 2021 13:14:49
An: Walter Harms; Colin King; Alex Deucher; David Airlie; Daniel Vetter; Huang Rui; Junwei Zhang; [email protected]; [email protected]
Cc: [email protected]; [email protected]
Betreff: Re: [PATCH] drm/amdgpu: fix potential integer overflow on shift of a int

For start and end? The hardware has 48 bit address space and that won't
fit into 32bits.

Only the fragment handling can't do more than 2GB at the same time.

Christian.

Am 08.02.21 um 12:05 schrieb Walter Harms:
> i am curious:
> what is the win to have a unsigned 64 bit integer in the first
> place ?
>
> re,
> wh
> ________________________________________
> Von: Christian K?nig <[email protected]>
> Gesendet: Montag, 8. Februar 2021 10:17:42
> An: Colin King; Alex Deucher; David Airlie; Daniel Vetter; Huang Rui; Junwei Zhang; [email protected]; [email protected]
> Cc: [email protected]; [email protected]
> Betreff: Re: [PATCH] drm/amdgpu: fix potential integer overflow on shift of a int
>
> Am 08.02.21 um 00:07 schrieb Colin King:
>> From: Colin Ian King <[email protected]>
>>
>> The left shift of int 32 bit integer constant 1 is evaluated using 32
>> bit arithmetic and then assigned to an unsigned 64 bit integer. In the
>> case where *frag is 32 or more this can lead to an oveflow. Avoid this
>> by shifting 1ULL.
> Well that can't happen. Take a look at the code in that function:
>
>> max_frag = 31;
> ...
>> if (*frag >= max_frag) {
>> *frag = max_frag;
>> *frag_end = end & ~((1ULL << max_frag) - 1);
>> } else {
>> *frag_end = start + (1 << *frag);
>> }
> But I'm fine with applying the patch if it silences your warning.
>
> Regards,
> Christian.
>
>> Addresses-Coverity: ("Unintentional integer overflow")
>> Fixes: dfcd99f6273e ("drm/amdgpu: meld together VM fragment and huge page handling")
>> Signed-off-by: Colin Ian King <[email protected]>
>> ---
>> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> index 9d19078246c8..53a925600510 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> @@ -1412,7 +1412,7 @@ static void amdgpu_vm_fragment(struct amdgpu_vm_update_params *params,
>> *frag = max_frag;
>> *frag_end = end & ~((1ULL << max_frag) - 1);
>> } else {
>> - *frag_end = start + (1 << *frag);
>> + *frag_end = start + (1ULL << *frag);
>> }
>> }
>>