Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH] media: media-request: Fix crash if memory allocation
 fails
To:     Tuomas Tynkkynen <tuomas.tynkkynen@iki.fi>, mchehab@kernel.org
Cc:     laurent.pinchart@ideasonboard.com, sakari.ailus@linux.intel.com,
        linux-media@vger.kernel.org, linux-kernel@vger.kernel.org,
        stable@vger.kernel.org
References: <20200621113040.3540-1-tuomas.tynkkynen@iki.fi>
From:   Hans Verkuil <hverkuil-cisco@xs4all.nl>
Message-ID: <06559806-3191-2e98-03d7-3ccfa8b7257c@xs4all.nl>
Date:   Mon, 22 Jun 2020 11:35:18 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.9.0
MIME-Version: 1.0
In-Reply-To: <20200621113040.3540-1-tuomas.tynkkynen@iki.fi>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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;
>  }
>  
>