Am 17.03.21 um 10:19 schrieb Chen Li:
On Wed, 17 Mar 2021 15:55:47 +0800,
Christian König wrote:
Am 17.03.21 um 07:22 schrieb Chen Li:
kvmalloc_array + __GFP_ZERO is the same with kvcalloc.
As for p->chunks, it will be used in:
```
if (ib_chunk->kdata)
memcpy(parser->ib.ptr, ib_chunk->kdata, ib_chunk->length_dw * 4);
```
If chunks doesn't zero out with __GFP_ZERO, it may point to somewhere else, e.g.,
```
Unable to handle kernel paging request at virtual address 0000000000010000
...
pc is at memcpy+0x84/0x250
ra is at radeon_cs_ioctl+0x368/0xb90 [radeon]
```
after allocating chunks with __GFP_KERNEL/kvcalloc, this bug is fixed.
NAK to zeroing the chunks array.
That array should be fully initialized with data before using it, otherwise we
have a much more serious bug and zeroing it out only papers over the real issue.
How did you trigger the NULL pointer deref above?
Hi, Christian, thanks for reply! From radeon_cs_parser_init:
```
if (user_chunk.chunk_id == RADEON_CHUNK_ID_IB) {
if (!p->rdev || !(p->rdev->flags & RADEON_IS_AGP))
/****** chenli: chunks[0] come here and continue! ******/
continue;
}
p->chunks[i].kdata = kvmalloc_array(size, sizeof(uint32_t), GFP_KERNEL);
```
In my case, chunks[0] is not allocated because it is just get continued, so it's not
wired that kdata in "memcpy(parser->ib.ptr, ib_chunk->kdata, ib_chunk->length_dw * 4);"
trigger the invalid address.
Right, the problem is this memory optimization added ~8 years ago.
We don't set the kdata pointer to NULL in that case, can you please add
this instead of setting the whole structure to zero?
Thanks,
Christian.
Thanks,
Christian.
Signed-off-by: Chen Li <chenli@xxxxxxxxxxxxx>
---
drivers/gpu/drm/radeon/radeon_cs.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/radeon/radeon_cs.c b/drivers/gpu/drm/radeon/radeon_cs.c
index fb736ef9f9aa..059431689c2d 100644
--- a/drivers/gpu/drm/radeon/radeon_cs.c
+++ b/drivers/gpu/drm/radeon/radeon_cs.c
@@ -93,8 +93,8 @@ static int radeon_cs_parser_relocs(struct radeon_cs_parser *p)
p->dma_reloc_idx = 0;
/* FIXME: we assume that each relocs use 4 dwords */
p->nrelocs = chunk->length_dw / 4;
- p->relocs = kvmalloc_array(p->nrelocs, sizeof(struct radeon_bo_list),
- GFP_KERNEL | __GFP_ZERO);
+ p->relocs = kvcalloc(p->nrelocs, sizeof(struct radeon_bo_list),
+ GFP_KERNEL);
if (p->relocs == NULL) {
return -ENOMEM;
}
@@ -299,7 +299,7 @@ int radeon_cs_parser_init(struct radeon_cs_parser *p, void *data)
}
p->cs_flags = 0;
p->nchunks = cs->num_chunks;
- p->chunks = kvmalloc_array(p->nchunks, sizeof(struct radeon_cs_chunk), GFP_KERNEL);
+ p->chunks = kvcalloc(p->nchunks, sizeof(struct radeon_cs_chunk), GFP_KERNEL);
if (p->chunks == NULL) {
return -ENOMEM;
}
Regards,
Chen Li
_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
