Re: [PATCH] media: media-request: Fix crash if memory allocation fails

On 21/06/2020 13:30, Tuomas Tynkkynen wrote:

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@syzkaller.appspotmail.com Cc: stable@vger.kernel.org Signed-off-by: Tuomas Tynkkynen tuomas.tynkkynen@iki.fi

Reviewed-by: Hans Verkuil hverkuil-cisco@xs4all.nl

Thanks!

Hans

---

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

diff --git a/drivers/media/mc/mc-request.c b/drivers/media/mc/mc-request.c index e3fca436c75b..c0782fd96c59 100644 --- a/drivers/media/mc/mc-request.c +++ b/drivers/media/mc/mc-request.c @@ -296,9 +296,18 @@ int media_request_alloc(struct media_device *mdev, int *alloc_fd) 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_device *mdev, int *alloc_fd) 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_device *mdev, int *alloc_fd) 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;

}