Extend the address and size validations to AMDGPU_VA_OP_UNMAP and
AMDGPU_VA_OP_CLEAR by moving the validations to amdgpu_gem_va_ioctl.
Internal users of amdgpu_vm_bo_map are no longer validated but they
should be fine.
Userspace (radeonsi and radv) seems fine as well.
---
drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 12 ++++++++++++
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 10 ----------
2 files changed, 12 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
index d8e683688daab..071f6565cf971 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
@@ -681,6 +681,18 @@ int amdgpu_gem_va_ioctl(struct drm_device *dev, void *data,
uint64_t vm_size;
int r = 0;
+ if (args->va_address & ~PAGE_MASK || args->offset_in_bo & ~PAGE_MASK ||
+ args->map_size & ~PAGE_MASK) {
+ dev_dbg(dev->dev, "unaligned va_address 0x%LX, offset_in_bo 0x%LX, or map_size 0x%LX\n",
+ args->va_address, args->offset_in_bo, args->map_size);
+ return -EINVAL;
+ }
+
+ if (args->map_size == 0) {
+ dev_dbg(dev->dev, "invalid map_size 0x%LX\n", args->map_size);
+ return -EINVAL;
+ }
+
if (args->va_address < AMDGPU_VA_RESERVED_SIZE) {
dev_dbg(dev->dev,
"va_address 0x%LX is in reserved area 0x%LX\n",
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index b9441ab457ea7..fa5819d581655 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -1435,11 +1435,6 @@ int amdgpu_vm_bo_map(struct amdgpu_device *adev,
struct amdgpu_vm *vm = bo_va->base.vm;
uint64_t eaddr;
- /* validate the parameters */
- if (saddr & ~PAGE_MASK || offset & ~PAGE_MASK ||
- size == 0 || size & ~PAGE_MASK)
- return -EINVAL;
-
/* make sure object fit at this offset */
eaddr = saddr + size - 1;
if (saddr >= eaddr ||
@@ -1501,11 +1496,6 @@ int amdgpu_vm_bo_replace_map(struct amdgpu_device *adev,
uint64_t eaddr;
int r;
- /* validate the parameters */
- if (saddr & ~PAGE_MASK || offset & ~PAGE_MASK ||
- size == 0 || size & ~PAGE_MASK)
- return -EINVAL;
-
/* make sure object fit at this offset */
eaddr = saddr + size - 1;
if (saddr >= eaddr ||
--
2.40.1.521.gf1e218fcd8-goog
The existing validations are incorrect and insufficient. This is
motivated by OOB access in amdgpu_vm_update_range when
offset_in_bo+map_size overflows.
Fixes: 9f7eb5367d00 ("drm/amdgpu: actually use the VM map parameters")
---
drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 7 +++++--
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 6 ++----
2 files changed, 7 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
index 071f6565cf971..36d5adfdf0f69 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
@@ -688,8 +688,11 @@ int amdgpu_gem_va_ioctl(struct drm_device *dev, void *data,
return -EINVAL;
}
- if (args->map_size == 0) {
- dev_dbg(dev->dev, "invalid map_size 0x%LX\n", args->map_size);
+ if (args->map_size == 0 ||
+ args->va_address + args->map_size < args->va_address ||
+ args->offset_in_bo + args->map_size < args->offset_in_bo) {
+ dev_dbg(dev->dev, "invalid map_size 0x%LX (va_address 0x%LX, offset_in_bo 0x%LX)\n",
+ args->map_size, args->va_address, args->offset_in_bo);
return -EINVAL;
}
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index fa5819d581655..cd0a0f06e11ef 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -1437,8 +1437,7 @@ int amdgpu_vm_bo_map(struct amdgpu_device *adev,
/* make sure object fit at this offset */
eaddr = saddr + size - 1;
- if (saddr >= eaddr ||
- (bo && offset + size > amdgpu_bo_size(bo)) ||
+ if ((bo && offset + size > amdgpu_bo_size(bo)) ||
(eaddr >= adev->vm_manager.max_pfn << AMDGPU_GPU_PAGE_SHIFT))
return -EINVAL;
@@ -1498,8 +1497,7 @@ int amdgpu_vm_bo_replace_map(struct amdgpu_device *adev,
/* make sure object fit at this offset */
eaddr = saddr + size - 1;
- if (saddr >= eaddr ||
- (bo && offset + size > amdgpu_bo_size(bo)) ||
+ if ((bo && offset + size > amdgpu_bo_size(bo)) ||
(eaddr >= adev->vm_manager.max_pfn << AMDGPU_GPU_PAGE_SHIFT))
return -EINVAL;
--
2.40.1.521.gf1e218fcd8-goog
On Tue, May 9, 2023 at 11:33 AM Chia-I Wu <[email protected]> wrote:
>
> Extend the address and size validations to AMDGPU_VA_OP_UNMAP and
> AMDGPU_VA_OP_CLEAR by moving the validations to amdgpu_gem_va_ioctl.
>
> Internal users of amdgpu_vm_bo_map are no longer validated but they
> should be fine.
>
> Userspace (radeonsi and radv) seems fine as well.
Does this series make sense?
On Wed, May 17, 2023 at 5:27 PM Chia-I Wu <[email protected]> wrote:
>
> On Tue, May 9, 2023 at 11:33 AM Chia-I Wu <[email protected]> wrote:
> >
> > Extend the address and size validations to AMDGPU_VA_OP_UNMAP and
> > AMDGPU_VA_OP_CLEAR by moving the validations to amdgpu_gem_va_ioctl.
> >
> > Internal users of amdgpu_vm_bo_map are no longer validated but they
> > should be fine.
> >
> > Userspace (radeonsi and radv) seems fine as well.
> Does this series make sense?
I think so, I haven't had a chance to go through this too closely yet,
but amdgpu_vm_bo_map() is used by ROCm as well so we'd need to make
sure that removing the checks in patch 1 wouldn't affect that path as
well. The changes in patch 2 look good. Also, these patches are
missing your SOB.
Thanks,
Alex
Alex
On Thu, May 18, 2023 at 1:12 PM Alex Deucher <[email protected]> wrote:
>
> On Wed, May 17, 2023 at 5:27 PM Chia-I Wu <[email protected]> wrote:
> >
> > On Tue, May 9, 2023 at 11:33 AM Chia-I Wu <[email protected]> wrote:
> > >
> > > Extend the address and size validations to AMDGPU_VA_OP_UNMAP and
> > > AMDGPU_VA_OP_CLEAR by moving the validations to amdgpu_gem_va_ioctl.
> > >
> > > Internal users of amdgpu_vm_bo_map are no longer validated but they
> > > should be fine.
> > >
> > > Userspace (radeonsi and radv) seems fine as well.
> > Does this series make sense?
>
> I think so, I haven't had a chance to go through this too closely yet,
> but amdgpu_vm_bo_map() is used by ROCm as well so we'd need to make
> sure that removing the checks in patch 1 wouldn't affect that path as
> well. The changes in patch 2 look good. Also, these patches are
> missing your SOB.
Indeed. kfd_ioctl_alloc_memory_of_gpu, for example, does not validate
va. I need to keep the validation in amdgpu_vm_bo_map for it at
least. I guess it is more ideal for kfd_ioctl_alloc_memory_of_gpu to
validate, but I am not familiar with amdkfd..
I can keep the existing validations, and duplicate them in
amdgpu_gem_va_ioctl to cover AMDGPU_VA_OP_UNMAP/AMDGPU_VA_OP_CLEAR.
>
> Thanks,
>
> Alex
>
>
> Alex
Am 21.05.23 um 20:49 schrieb Chia-I Wu:
> On Thu, May 18, 2023 at 1:12 PM Alex Deucher <[email protected]> wrote:
>> On Wed, May 17, 2023 at 5:27 PM Chia-I Wu <[email protected]> wrote:
>>> On Tue, May 9, 2023 at 11:33 AM Chia-I Wu <[email protected]> wrote:
>>>> Extend the address and size validations to AMDGPU_VA_OP_UNMAP and
>>>> AMDGPU_VA_OP_CLEAR by moving the validations to amdgpu_gem_va_ioctl.
>>>>
>>>> Internal users of amdgpu_vm_bo_map are no longer validated but they
>>>> should be fine.
>>>>
>>>> Userspace (radeonsi and radv) seems fine as well.
>>> Does this series make sense?
>> I think so, I haven't had a chance to go through this too closely yet,
>> but amdgpu_vm_bo_map() is used by ROCm as well so we'd need to make
>> sure that removing the checks in patch 1 wouldn't affect that path as
>> well. The changes in patch 2 look good. Also, these patches are
>> missing your SOB.
> Indeed. kfd_ioctl_alloc_memory_of_gpu, for example, does not validate
> va. I need to keep the validation in amdgpu_vm_bo_map for it at
> least. I guess it is more ideal for kfd_ioctl_alloc_memory_of_gpu to
> validate, but I am not familiar with amdkfd..
>
> I can keep the existing validations, and duplicate them in
> amdgpu_gem_va_ioctl to cover AMDGPU_VA_OP_UNMAP/AMDGPU_VA_OP_CLEAR.
The key point is that unmap and clear don't need those validations.
It's perfectly valid to request unmap of an unaligned mapping, it will
just fail because we can't find that mapping.
Regards,
Christian.
>
>> Thanks,
>>
>> Alex
>>
>>
>> Alex
On Mon, May 22, 2023 at 12:12 PM Christian König
<[email protected]> wrote:
>
> Am 21.05.23 um 20:49 schrieb Chia-I Wu:
> > On Thu, May 18, 2023 at 1:12 PM Alex Deucher <[email protected]> wrote:
> >> On Wed, May 17, 2023 at 5:27 PM Chia-I Wu <[email protected]> wrote:
> >>> On Tue, May 9, 2023 at 11:33 AM Chia-I Wu <[email protected]> wrote:
> >>>> Extend the address and size validations to AMDGPU_VA_OP_UNMAP and
> >>>> AMDGPU_VA_OP_CLEAR by moving the validations to amdgpu_gem_va_ioctl.
> >>>>
> >>>> Internal users of amdgpu_vm_bo_map are no longer validated but they
> >>>> should be fine.
> >>>>
> >>>> Userspace (radeonsi and radv) seems fine as well.
> >>> Does this series make sense?
> >> I think so, I haven't had a chance to go through this too closely yet,
> >> but amdgpu_vm_bo_map() is used by ROCm as well so we'd need to make
> >> sure that removing the checks in patch 1 wouldn't affect that path as
> >> well. The changes in patch 2 look good. Also, these patches are
> >> missing your SOB.
> > Indeed. kfd_ioctl_alloc_memory_of_gpu, for example, does not validate
> > va. I need to keep the validation in amdgpu_vm_bo_map for it at
> > least. I guess it is more ideal for kfd_ioctl_alloc_memory_of_gpu to
> > validate, but I am not familiar with amdkfd..
> >
> > I can keep the existing validations, and duplicate them in
> > amdgpu_gem_va_ioctl to cover AMDGPU_VA_OP_UNMAP/AMDGPU_VA_OP_CLEAR.
>
> The key point is that unmap and clear don't need those validations.
>
> It's perfectly valid to request unmap of an unaligned mapping, it will
> just fail because we can't find that mapping.
unmap and clear_mappings convert addresses to gpu pages so unaligned
addresses are treated as if they were aligned. That's likely fine
except that might be an unintentional inconsistency between va ops?
When args->map_size is 0, eaddr can be smaller than saddr in
clear_mappings. We are also at the mercy of how interval trees are
implemented.
>
> Regards,
> Christian.
>
> >
> >> Thanks,
> >>
> >> Alex
> >>
> >>
> >> Alex
>