[syzbot] [dri?] KMSAN: uninit-value in drm_mode_setcrtc

[syzbot]

Hello,

syzbot found the following issue on:

HEAD commit: 2741f1b02117 string: use __builtin_memcpy() in strlcpy/str..
git tree: https://github.com/google/kmsan.git master
console+strace: https://syzkaller.appspot.com/x/log.txt?x=17bb33d1280000
kernel config: https://syzkaller.appspot.com/x/.config?x=753079601b2300f9
dashboard link: https://syzkaller.appspot.com/bug?extid=4fad2e57beb6397ab2fc
compiler: Debian clang version 15.0.7, GNU ld (GNU Binutils for Debian) 2.35.2
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=16d669a5280000
C reproducer: https://syzkaller.appspot.com/x/repro.c?x=14d8f095280000

Downloadable assets:
disk image: https://storage.googleapis.com/syzbot-assets/ebd05512d8d7/disk-2741f1b0.raw.xz
vmlinux: https://storage.googleapis.com/syzbot-assets/aa555b09582c/vmlinux-2741f1b0.xz
kernel image: https://storage.googleapis.com/syzbot-assets/5ea0934e02cc/bzImage-2741f1b0.xz

IMPORTANT: if you fix the issue, please add the following tag to the commit:
Reported-by: syzbot+4fad2e...@syzkaller.appspotmail.com

=====================================================
BUG: KMSAN: uninit-value in drm_mode_setcrtc+0x1ad3/0x24a0 drivers/gpu/drm/drm_crtc.c:896
drm_mode_setcrtc+0x1ad3/0x24a0 drivers/gpu/drm/drm_crtc.c:896
drm_ioctl_kernel+0x5ae/0x730 drivers/gpu/drm/drm_ioctl.c:788
drm_ioctl+0xd12/0x1590 drivers/gpu/drm/drm_ioctl.c:891
vfs_ioctl fs/ioctl.c:51 [inline]
__do_sys_ioctl fs/ioctl.c:870 [inline]
__se_sys_ioctl+0x222/0x400 fs/ioctl.c:856
__x64_sys_ioctl+0x96/0xe0 fs/ioctl.c:856
do_syscall_x64 arch/x86/entry/common.c:50 [inline]
do_syscall_64+0x41/0xc0 arch/x86/entry/common.c:80
entry_SYSCALL_64_after_hwframe+0x63/0xcd

Uninit was created at:
slab_post_alloc_hook+0x12d/0xb60 mm/slab.h:716
slab_alloc_node mm/slub.c:3451 [inline]
__kmem_cache_alloc_node+0x4ff/0x8b0 mm/slub.c:3490
__do_kmalloc_node mm/slab_common.c:965 [inline]
__kmalloc+0x121/0x3c0 mm/slab_common.c:979
kmalloc_array include/linux/slab.h:596 [inline]
drm_mode_setcrtc+0x1dba/0x24a0 drivers/gpu/drm/drm_crtc.c:846
drm_ioctl_kernel+0x5ae/0x730 drivers/gpu/drm/drm_ioctl.c:788
drm_ioctl+0xd12/0x1590 drivers/gpu/drm/drm_ioctl.c:891
vfs_ioctl fs/ioctl.c:51 [inline]
__do_sys_ioctl fs/ioctl.c:870 [inline]
__se_sys_ioctl+0x222/0x400 fs/ioctl.c:856
__x64_sys_ioctl+0x96/0xe0 fs/ioctl.c:856
do_syscall_x64 arch/x86/entry/common.c:50 [inline]
do_syscall_64+0x41/0xc0 arch/x86/entry/common.c:80
entry_SYSCALL_64_after_hwframe+0x63/0xcd

CPU: 1 PID: 4955 Comm: syz-executor275 Not tainted 6.4.0-rc4-syzkaller-g2741f1b02117 #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 05/25/2023
=====================================================


---
This report is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzk...@googlegroups.com.

syzbot will keep track of this issue. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.

If the bug is already fixed, let syzbot know by replying with:
#syz fix: exact-commit-title

If you want syzbot to run the reproducer, reply with:
#syz test: git://repo/address.git branch-or-commit-hash
If you attach or paste a git patch, syzbot will apply it before testing.

If you want to change bug's subsystems, reply with:
#syz set subsystems: new-subsystem
(See the list of subsystem names on the web dashboard)

If the bug is a duplicate of another bug, reply with:
#syz dup: exact-subject-of-another-report

If you want to undo deduplication, reply with:
#syz undup

[Ziqi Zhao]

The connector_set contains uninitialized values when allocated with
kmalloc_array. However, in the "out" branch, the logic assumes that any
element in connector_set would be equal to NULL if failed to
initialize, which causes the bug reported by Syzbot. The fix is to use
an extra variable to keep track of how many connectors are initialized
indeed, and use that variable to decrease any refcounts in the "out"
branch.

#syz test: https://github.com/google/kmsan.git master

Reported-by: syzbot+4fad2e...@syzkaller.appspotmail.com
Signed-off-by: Ziqi Zhao <astr...@yahoo.com>
---
drivers/gpu/drm/drm_crtc.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index df9bf3c9206e..d718c17ab1e9 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -715,8 +715,7 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
struct drm_mode_set set;
uint32_t __user *set_connectors_ptr;
struct drm_modeset_acquire_ctx ctx;
- int ret;
- int i;
+ int ret, i, num_connectors;

if (!drm_core_check_feature(dev, DRIVER_MODESET))
return -EOPNOTSUPP;
@@ -851,6 +850,7 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
goto out;
}

+ num_connectors = 0;
for (i = 0; i < crtc_req->count_connectors; i++) {
connector_set[i] = NULL;
set_connectors_ptr = (uint32_t __user *)(unsigned long)crtc_req->set_connectors_ptr;
@@ -871,6 +871,7 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
connector->name);

connector_set[i] = connector;
+ num_connectors++;
}
}

@@ -879,7 +880,7 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
set.y = crtc_req->y;
set.mode = mode;
set.connectors = connector_set;
- set.num_connectors = crtc_req->count_connectors;
+ set.num_connectors = num_connectors;
set.fb = fb;

if (drm_drv_uses_atomic_modeset(dev))
@@ -892,7 +893,7 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
drm_framebuffer_put(fb);

if (connector_set) {
- for (i = 0; i < crtc_req->count_connectors; i++) {
+ for (i = 0; i < num_connectors; i++) {
if (connector_set[i])
drm_connector_put(connector_set[i]);
}
--
2.34.1

[syzbot]

Hello,

syzbot has tested the proposed patch and the reproducer did not trigger any issue:

Reported-and-tested-by: syzbot+4fad2e...@syzkaller.appspotmail.com

Tested on:

commit: d1d7f15c DO-NOT-SUBMIT: kmsan: add the kmsan_exceed_ma..

git tree: https://github.com/google/kmsan.git master

console output: https://syzkaller.appspot.com/x/log.txt?x=101d3fdaa80000
kernel config: https://syzkaller.appspot.com/x/.config?x=36e4a2f8150fbc62
dashboard link: https://syzkaller.appspot.com/bug?extid=4fad2e57beb6397ab2fc
compiler: Debian clang version 15.0.6, GNU ld (GNU Binutils for Debian) 2.40
patch: https://syzkaller.appspot.com/x/patch.diff?x=15430342a80000

Note: testing is done by a robot and is best-effort only.

[Ziqi Zhao]

The connector_set contains uninitialized values when allocated with
kmalloc_array. However, in the "out" branch, the logic assumes that any
element in connector_set would be equal to NULL if failed to
initialize, which causes the bug reported by Syzbot. The fix is to use
an extra variable to keep track of how many connectors are initialized
indeed, and use that variable to decrease any refcounts in the "out"
branch.

[Maxime Ripard]

On Fri, 21 Jul 2023 09:14:46 -0700, Ziqi Zhao wrote:
> The connector_set contains uninitialized values when allocated with
> kmalloc_array. However, in the "out" branch, the logic assumes that any
> element in connector_set would be equal to NULL if failed to
> initialize, which causes the bug reported by Syzbot. The fix is to use
> an extra variable to keep track of how many connectors are initialized
> indeed, and use that variable to decrease any refcounts in the "out"
> branch.
>

> [...]

Applied to drm/drm-misc (drm-misc-fixes).

Thanks!
Maxime

[Harshit Mogalapalli]

Hello,

On 21/07/23 9:44 pm, Ziqi Zhao wrote:
> The connector_set contains uninitialized values when allocated with
> kmalloc_array. However, in the "out" branch, the logic assumes that any
> element in connector_set would be equal to NULL if failed to
> initialize, which causes the bug reported by Syzbot. The fix is to use
> an extra variable to keep track of how many connectors are initialized
> indeed, and use that variable to decrease any refcounts in the "out"
> branch.
>

This bug is reproducible on 6.7-rc3 on KASAN enabled kernel as wild
memory access.

[ 424.699429] general protection fault, probably for non-canonical
address 0xfbf7c8b63d84d2a6: 0000 [#1] PREEMPT SMP KASAN PTI
[ 424.727952] KASAN: maybe wild-memory-access in range
[0xdfbe65b1ec269530-0xdfbe65b1ec269537]
[ 424.743794] CPU: 3 PID: 9040 Comm: r Not tainted 6.7.0-rc3+ #1
[ 424.758855] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
BIOS 1.11.0-2.el7 04/01/2014
[ 424.774845] RIP: 0010:drm_mode_object_put+0x27/0x50
[ 424.782854] Code: 90 90 90 f3 0f 1e fa 0f 1f 44 00 00 55 48 89 fd e8
ae 92 0b fd 48 8d 7d 18 48 b8 00 00 00 00 00 fc ff df 48 89 fa 48 c1 ea
03 <80> 3c 02 00 75 1a 48 83 7d 18 00 74 0d e8 87 92 0b fd 48 89 ef e8
[ 424.816805] RSP: 0018:ffff8881199b7ad0 EFLAGS: 00010a06
[ 424.830847] RAX: dffffc0000000000 RBX: ffffed1023336fc3 RCX:
0000000000000000
[ 424.844180] RDX: 1bf7ccb63d84d2a6 RSI: 0000000000000000 RDI:
dfbe65b1ec269530
[ 424.854860] RBP: dfbe65b1ec269518 R08: 0000000000000000 R09:
0000000000000000
[ 424.870833] R10: 0000000000000000 R11: 0000000000000000 R12:
dfbe65b1ec2694d8
[ 424.886846] R13: dffffc0000000000 R14: ffff8881060731c0 R15:
0000000000000001
[ 424.901889] FS: 00007fecfc1ec740(0000) GS:ffff8881f3f80000(0000)
knlGS:0000000000000000
[ 424.910833] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 424.918929] CR2: 0000000000000000 CR3: 0000000117e7c000 CR4:
00000000000006f0
[ 424.936058] Call Trace:
[ 424.936058] <TASK>
[ 424.936058] ? show_regs+0x9b/0xb0
[ 424.950853] ? die_addr+0x55/0xe0
[ 424.950853] ? exc_general_protection+0x1a4/0x320
[ 424.965905] ? asm_exc_general_protection+0x26/0x30
[ 424.974878] ? drm_mode_object_put+0x27/0x50
[ 424.982866] drm_mode_setcrtc+0x7ec/0x1630
[ 424.990875] ? __pfx_drm_mode_setcrtc+0x10/0x10
[ 424.998877] ? ww_mutex_lock+0x9a/0x1c0
[ 425.006852] ? __pfx_ww_mutex_lock+0x10/0x10
[ 425.014875] ? __drm_dev_dbg+0xbd/0x1a0
[ 425.014875] ? __pfx___drm_dev_dbg+0x10/0x10
[ 425.030321] ? drm_lease_owner+0x44/0x60
[ 425.030981] drm_ioctl_kernel+0x2a0/0x500
[ 425.040058] ? __pfx_drm_mode_setcrtc+0x10/0x10
[ 425.048128] ? __pfx_drm_ioctl_kernel+0x10/0x10
[ 425.055809] drm_ioctl+0x58a/0xb60
[ 425.062876] ? __pfx_drm_mode_setcrtc+0x10/0x10
[ 425.070875] ? __pfx_drm_ioctl+0x10/0x10
[ 425.078875] ? __pfx_do_sys_openat2+0x10/0x10
[ 425.086875] ? selinux_file_ioctl+0x184/0x270
[ 425.099093] ? selinux_file_ioctl+0xba/0x270
[ 425.102865] ? __pfx_drm_ioctl+0x10/0x10
[ 425.111092] __x64_sys_ioctl+0x1b1/0x220
[ 425.119055] do_syscall_64+0x45/0x100
[ 425.127106] entry_SYSCALL_64_after_hwframe+0x6e/0x76
[ 425.135102] RIP: 0033:0x7fecfb6f8289


After applying this patch, the bug is not reproducible.


Thanks,
Harshit

[Jani Nikula]

On Fri, 21 Jul 2023, Ziqi Zhao <astr...@yahoo.com> wrote:
> The connector_set contains uninitialized values when allocated with
> kmalloc_array. However, in the "out" branch, the logic assumes that any
> element in connector_set would be equal to NULL if failed to
> initialize, which causes the bug reported by Syzbot. The fix is to use
> an extra variable to keep track of how many connectors are initialized
> indeed, and use that variable to decrease any refcounts in the "out"
> branch.

From one uninit-value bug to another?

>
> Reported-by: syzbot+4fad2e...@syzkaller.appspotmail.com
> Signed-off-by: Ziqi Zhao <astr...@yahoo.com>
> ---
> drivers/gpu/drm/drm_crtc.c | 9 +++++----
> 1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index df9bf3c9206e..d718c17ab1e9 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -715,8 +715,7 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
> struct drm_mode_set set;
> uint32_t __user *set_connectors_ptr;
> struct drm_modeset_acquire_ctx ctx;
> - int ret;
> - int i;
> + int ret, i, num_connectors;

num_connectors is uninitialized.

>
> if (!drm_core_check_feature(dev, DRIVER_MODESET))
> return -EOPNOTSUPP;
> @@ -851,6 +850,7 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
> goto out;
> }
>
> + num_connectors = 0;

num_connectors is initialized only if crtc_req->count_connectors > 0.

> for (i = 0; i < crtc_req->count_connectors; i++) {
> connector_set[i] = NULL;
> set_connectors_ptr = (uint32_t __user *)(unsigned long)crtc_req->set_connectors_ptr;
> @@ -871,6 +871,7 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
> connector->name);
>
> connector_set[i] = connector;
> + num_connectors++;
> }
> }
>
> @@ -879,7 +880,7 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
> set.y = crtc_req->y;
> set.mode = mode;
> set.connectors = connector_set;
> - set.num_connectors = crtc_req->count_connectors;
> + set.num_connectors = num_connectors;

num_connectors is used uninitialized if crtc_req->count_connectors <= 0.

BR,
Jani.

> set.fb = fb;
>
> if (drm_drv_uses_atomic_modeset(dev))
> @@ -892,7 +893,7 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
> drm_framebuffer_put(fb);
>
> if (connector_set) {
> - for (i = 0; i < crtc_req->count_connectors; i++) {
> + for (i = 0; i < num_connectors; i++) {
> if (connector_set[i])
> drm_connector_put(connector_set[i]);
> }

--

Jani Nikula, Intel