syzbot reported GPF in dma_fence_array_first(), which is caused by dereferencing ZERO_PTR in dma-buf internals.
ZERO_PTR was generated in sync_file_merge(). This functuion tries to reduce allocation size, but does not check if it reducing to 0.
Fix reported bug by validating `index` value before passing it to krealloc_array().
Fail log:
general protection fault, probably for non-canonical address 0xdffffc0000000002: 0000 [#1] PREEMPT SMP KASAN KASAN: null-ptr-deref in range [0x0000000000000010-0x0000000000000017] CPU: 1 PID: 3595 Comm: syz-executor814 Not tainted 5.17.0-next-20220328-syzkaller #0 ... RIP: 0010:dma_fence_array_first+0x78/0xb0 drivers/dma-buf/dma-fence-array.c:234 ... Call Trace: <TASK> __dma_fence_unwrap_array include/linux/dma-fence-unwrap.h:42 [inline] dma_fence_unwrap_first include/linux/dma-fence-unwrap.h:57 [inline] sync_file_ioctl_fence_info drivers/dma-buf/sync_file.c:414 [inline] sync_file_ioctl+0x248/0x22c0 drivers/dma-buf/sync_file.c:477 vfs_ioctl fs/ioctl.c:51 [inline] __do_sys_ioctl fs/ioctl.c:870 [inline]
There was same problem with initial kcalloc() allocation in same function, so it's fixed as well.
Reported-and-tested-by: syzbot+5c943fe38e86d615cac2@syzkaller.appspotmail.com Fixes: 519f490db07e ("dma-buf/sync-file: fix warning about fence containers") Signed-off-by: Pavel Skripkin paskripkin@gmail.com --- drivers/dma-buf/sync_file.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c index b8dea4ec123b..aa744f017008 100644 --- a/drivers/dma-buf/sync_file.c +++ b/drivers/dma-buf/sync_file.c @@ -212,7 +212,7 @@ static struct sync_file *sync_file_merge(const char *name, struct sync_file *a, dma_fence_unwrap_for_each(b_fence, &b_iter, b->fence) ++num_fences;
- if (num_fences > INT_MAX) + if (num_fences > INT_MAX || !num_fences) goto err_free_sync_file;
fences = kcalloc(num_fences, sizeof(*fences), GFP_KERNEL); @@ -264,7 +264,7 @@ static struct sync_file *sync_file_merge(const char *name, struct sync_file *a, if (index == 0) add_fence(fences, &index, dma_fence_get_stub());
- if (num_fences > index) { + if (index && num_fences > index) { struct dma_fence **tmp;
/* Keep going even when reducing the size failed */
That problem is already fixed with patch 21d139d73f77 dma-buf/sync-file: fix logic error in new fence merge code.
Am 30.03.22 um 00:14 schrieb Pavel Skripkin:
syzbot reported GPF in dma_fence_array_first(), which is caused by dereferencing ZERO_PTR in dma-buf internals.
ZERO_PTR was generated in sync_file_merge(). This functuion tries to reduce allocation size, but does not check if it reducing to 0.
This is actually perfectly ok. The code above should have prevented the size to become 0.
Regards, Christian.
Fix reported bug by validating `index` value before passing it to krealloc_array().
Fail log:
general protection fault, probably for non-canonical address 0xdffffc0000000002: 0000 [#1] PREEMPT SMP KASAN KASAN: null-ptr-deref in range [0x0000000000000010-0x0000000000000017] CPU: 1 PID: 3595 Comm: syz-executor814 Not tainted 5.17.0-next-20220328-syzkaller #0 ... RIP: 0010:dma_fence_array_first+0x78/0xb0 drivers/dma-buf/dma-fence-array.c:234 ... Call Trace:
<TASK> __dma_fence_unwrap_array include/linux/dma-fence-unwrap.h:42 [inline] dma_fence_unwrap_first include/linux/dma-fence-unwrap.h:57 [inline] sync_file_ioctl_fence_info drivers/dma-buf/sync_file.c:414 [inline] sync_file_ioctl+0x248/0x22c0 drivers/dma-buf/sync_file.c:477 vfs_ioctl fs/ioctl.c:51 [inline] __do_sys_ioctl fs/ioctl.c:870 [inline]
There was same problem with initial kcalloc() allocation in same function, so it's fixed as well.
Reported-and-tested-by: syzbot+5c943fe38e86d615cac2@syzkaller.appspotmail.com Fixes: 519f490db07e ("dma-buf/sync-file: fix warning about fence containers") Signed-off-by: Pavel Skripkin paskripkin@gmail.com
drivers/dma-buf/sync_file.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c index b8dea4ec123b..aa744f017008 100644 --- a/drivers/dma-buf/sync_file.c +++ b/drivers/dma-buf/sync_file.c @@ -212,7 +212,7 @@ static struct sync_file *sync_file_merge(const char *name, struct sync_file *a, dma_fence_unwrap_for_each(b_fence, &b_iter, b->fence) ++num_fences;
- if (num_fences > INT_MAX)
if (num_fences > INT_MAX || !num_fences) goto err_free_sync_file;
fences = kcalloc(num_fences, sizeof(*fences), GFP_KERNEL);
@@ -264,7 +264,7 @@ static struct sync_file *sync_file_merge(const char *name, struct sync_file *a, if (index == 0) add_fence(fences, &index, dma_fence_get_stub());
- if (num_fences > index) {
if (index && num_fences > index) { struct dma_fence **tmp;
/* Keep going even when reducing the size failed */
Hi Christian,
On 3/30/22 10:09, Christian König wrote:
That problem is already fixed with patch 21d139d73f77 dma-buf/sync-file: fix logic error in new fence merge code.
Am 30.03.22 um 00:14 schrieb Pavel Skripkin:
syzbot reported GPF in dma_fence_array_first(), which is caused by dereferencing ZERO_PTR in dma-buf internals.
ZERO_PTR was generated in sync_file_merge(). This functuion tries to reduce allocation size, but does not check if it reducing to 0.
This is actually perfectly ok. The code above should have prevented the size to become 0.
Regards, Christian.
Thanks for your reply! I see that 21d139d73f77 fixes GPF in dma_fence_array_first(), but what about this part:
- if (num_fences > INT_MAX)
if (num_fences > INT_MAX || !num_fences) goto err_free_sync_file;
fences = kcalloc(num_fences, sizeof(*fences), GFP_KERNEL);
@@ -264,7 +264,7 @@ static struct sync_file *sync_file_merge(const char *name, struct sync_file *a, if (index == 0)
If num_fences is equal to zero then fences dereference will cause an oops. Or this one is also fixed in your tree?
Thanks!
With regards, Pavel Skripkin
Am 30.03.22 um 20:24 schrieb Pavel Skripkin:
Hi Christian,
On 3/30/22 10:09, Christian König wrote:
That problem is already fixed with patch 21d139d73f77 dma-buf/sync-file: fix logic error in new fence merge code.
Am 30.03.22 um 00:14 schrieb Pavel Skripkin:
syzbot reported GPF in dma_fence_array_first(), which is caused by dereferencing ZERO_PTR in dma-buf internals.
ZERO_PTR was generated in sync_file_merge(). This functuion tries to reduce allocation size, but does not check if it reducing to 0.
This is actually perfectly ok. The code above should have prevented the size to become 0.
Regards, Christian.
Thanks for your reply! I see that 21d139d73f77 fixes GPF in dma_fence_array_first(), but what about this part:
- if (num_fences > INT_MAX) + if (num_fences > INT_MAX || !num_fences) goto err_free_sync_file; fences = kcalloc(num_fences, sizeof(*fences), GFP_KERNEL); @@ -264,7 +264,7 @@ static struct sync_file *sync_file_merge(const char *name, struct sync_file *a, if (index == 0)
If num_fences is equal to zero then fences dereference will cause an oops. Or this one is also fixed in your tree?
Well it is illegal for sync_file->fence to be NULL or we would run into NULL pointer dereference much more often, so num_fences can't be zero here either.
Regards, Christian.
Thanks!
With regards, Pavel Skripkin
num_fences is user-controlled value and it can be equal to 0. Code should not pass 0 to kcalloc(), since it will cause kcalloc() to return ZERO_PTR. ZERO_PTR will pass `!fences` check and kernel will panic because of dereferencing ZERO_PTR in add_fence()
Fix it by validating num_fences and bail out early if it is equal to 0
Fixes: 519f490db07e ("dma-buf/sync-file: fix warning about fence containers") Signed-off-by: Pavel Skripkin paskripkin@gmail.com ---
Changes since v1: - Dropped already merged part - Removed syzkaller's tag
--- drivers/dma-buf/sync_file.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c index b8dea4ec123b..024f22193e0c 100644 --- a/drivers/dma-buf/sync_file.c +++ b/drivers/dma-buf/sync_file.c @@ -212,7 +212,7 @@ static struct sync_file *sync_file_merge(const char *name, struct sync_file *a, dma_fence_unwrap_for_each(b_fence, &b_iter, b->fence) ++num_fences;
- if (num_fences > INT_MAX) + if (num_fences > INT_MAX || !num_fences) goto err_free_sync_file;
fences = kcalloc(num_fences, sizeof(*fences), GFP_KERNEL);
Am 01.04.22 um 23:31 schrieb Pavel Skripkin:
num_fences is user-controlled value and it can be equal to 0. Code should not pass 0 to kcalloc(), since it will cause kcalloc() to return ZERO_PTR. ZERO_PTR will pass `!fences` check and kernel will panic because of dereferencing ZERO_PTR in add_fence()
Fix it by validating num_fences and bail out early if it is equal to 0
Well there are multiple issues with this patch. First of all as I wrote before it shouldn't be possible that num_fences is zero.
We could still just add this as a precaution, but then bailing out is the wrong thing to do here. Instead we should then make sure to allocate at least one slot for a fence in the array.
But I think the cleanest would just be to not add a fence into the array in the first place when num_fences is zero.
Regards, Christian.
Fixes: 519f490db07e ("dma-buf/sync-file: fix warning about fence containers") Signed-off-by: Pavel Skripkin paskripkin@gmail.com
Changes since v1:
- Dropped already merged part
- Removed syzkaller's tag
drivers/dma-buf/sync_file.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c index b8dea4ec123b..024f22193e0c 100644 --- a/drivers/dma-buf/sync_file.c +++ b/drivers/dma-buf/sync_file.c @@ -212,7 +212,7 @@ static struct sync_file *sync_file_merge(const char *name, struct sync_file *a, dma_fence_unwrap_for_each(b_fence, &b_iter, b->fence) ++num_fences;
- if (num_fences > INT_MAX)
if (num_fences > INT_MAX || !num_fences) goto err_free_sync_file;
fences = kcalloc(num_fences, sizeof(*fences), GFP_KERNEL);
dri-devel@lists.freedesktop.org
-
Christian König -
Christian König -
Pavel Skripkin