linux-kernel - [PATCH 5.8 422/464] media: media-request: Fix crash if memory allocation fails

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20200817143853.988809228@linuxfoundation.org>
Date:   Mon, 17 Aug 2020 17:16:15 +0200
From:   Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To:     linux-kernel@...r.kernel.org
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        stable@...r.kernel.org,
        syzbot+6bed2d543cf7e48b822b@...kaller.appspotmail.com,
        Tuomas Tynkkynen <tuomas.tynkkynen@....fi>,
        Hans Verkuil <hverkuil-cisco@...all.nl>,
        Sakari Ailus <sakari.ailus@...ux.intel.com>,
        Mauro Carvalho Chehab <mchehab+huawei@...nel.org>
Subject: [PATCH 5.8 422/464] media: media-request: Fix crash if memory allocation fails

From: Tuomas Tynkkynen <tuomas.tynkkynen@....fi>

commit e30cc79cc80fd919b697a15c5000d9f57487de8e upstream.

Syzbot reports a NULL-ptr deref in the kref_put() call:

BUG: KASAN: null-ptr-deref in media_request_put drivers/media/mc/mc-request.c:81 [inline]
 kref_put include/linux/kref.h:64 [inline]
 media_request_put drivers/media/mc/mc-request.c:81 [inline]
 media_request_close+0x4d/0x170 drivers/media/mc/mc-request.c:89
 __fput+0x2ed/0x750 fs/file_table.c:281
 task_work_run+0x147/0x1d0 kernel/task_work.c:123
 tracehook_notify_resume include/linux/tracehook.h:188 [inline]
 exit_to_usermode_loop arch/x86/entry/common.c:165 [inline]
 prepare_exit_to_usermode+0x48e/0x600 arch/x86/entry/common.c:196

What led to this crash was an injected memory allocation failure in
media_request_alloc():

FAULT_INJECTION: forcing a failure.
name failslab, interval 1, probability 0, space 0, times 0
 should_failslab+0x5/0x20
 kmem_cache_alloc_trace+0x57/0x300
 ? anon_inode_getfile+0xe5/0x170
 media_request_alloc+0x339/0x440
 media_device_request_alloc+0x94/0xc0
 media_device_ioctl+0x1fb/0x330
 ? do_vfs_ioctl+0x6ea/0x1a00
 ? media_ioctl+0x101/0x120
 ? __media_device_usb_init+0x430/0x430
 ? media_poll+0x110/0x110
 __se_sys_ioctl+0xf9/0x160
 do_syscall_64+0xf3/0x1b0

When that allocation fails, filp->private_data is left uninitialized
which media_request_close() does not expect and crashes.

To avoid this, reorder media_request_alloc() such that
allocating the struct file happens as the last step thus
media_request_close() will no longer get called for a partially created
media request.

Reported-by: syzbot+6bed2d543cf7e48b822b@...kaller.appspotmail.com
Cc: stable@...r.kernel.org
Signed-off-by: Tuomas Tynkkynen <tuomas.tynkkynen@....fi>
Fixes: 10905d70d788 ("media: media-request: implement media requests")
Reviewed-by: Hans Verkuil <hverkuil-cisco@...all.nl>
Signed-off-by: Sakari Ailus <sakari.ailus@...ux.intel.com>
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@...nel.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@...uxfoundation.org>

---
 drivers/media/mc/mc-request.c |   31 +++++++++++++++++--------------
 1 file changed, 17 insertions(+), 14 deletions(-)

--- a/drivers/media/mc/mc-request.c
+++ b/drivers/media/mc/mc-request.c
@@ -296,9 +296,18 @@ int media_request_alloc(struct media_dev
 	if (WARN_ON(!mdev->ops->req_alloc ^ !mdev->ops->req_free))
 		return -ENOMEM;
 
+	if (mdev->ops->req_alloc)
+		req = mdev->ops->req_alloc(mdev);
+	else
+		req = kzalloc(sizeof(*req), GFP_KERNEL);
+	if (!req)
+		return -ENOMEM;
+
 	fd = get_unused_fd_flags(O_CLOEXEC);
-	if (fd < 0)
-		return fd;
+	if (fd < 0) {
+		ret = fd;
+		goto err_free_req;
+	}
 
 	filp = anon_inode_getfile("request", &request_fops, NULL, O_CLOEXEC);
 	if (IS_ERR(filp)) {
@@ -306,15 +315,6 @@ int media_request_alloc(struct media_dev
 		goto err_put_fd;
 	}
 
-	if (mdev->ops->req_alloc)
-		req = mdev->ops->req_alloc(mdev);
-	else
-		req = kzalloc(sizeof(*req), GFP_KERNEL);
-	if (!req) {
-		ret = -ENOMEM;
-		goto err_fput;
-	}
-
 	filp->private_data = req;
 	req->mdev = mdev;
 	req->state = MEDIA_REQUEST_STATE_IDLE;
@@ -336,12 +336,15 @@ int media_request_alloc(struct media_dev
 
 	return 0;
 
-err_fput:
-	fput(filp);
-
 err_put_fd:
 	put_unused_fd(fd);
 
+err_free_req:
+	if (mdev->ops->req_free)
+		mdev->ops->req_free(req);
+	else
+		kfree(req);
+
 	return ret;
 }