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] [day] [month] [year] [list]
Date:   Mon, 7 Nov 2022 17:16:33 +0200
From:   "Martin Krastev (VMware)" <martinkrastev768@...il.com>
To:     Dawei Li <set_pte_at@...look.com>, zackr@...are.com,
        airlied@...il.com, daniel@...ll.ch
Cc:     krastevm@...are.com, linux-graphics-maintainer@...are.com,
        linux-kernel@...r.kernel.org, dri-devel@...ts.freedesktop.org
Subject: Re: [PATCH v2] drm/vmwgfx: Protect pin_user_pages with mmap_lock

From: Martin Krastev <krastevm@...are.com>



Thanks for the catch. LGTM, with the following remarks:


1) Original design used erroneously pin_user_pages() in place of 
pin_user_pages_fast(); you could just substitute pin_user_pages for 
pin_user_pages_fast and call it a day, Please, consider that option 
after reading (2) below.

2) Re exception handling in vmw_mksstat_add_ioctl(), the 'incorrect 
exception handling' would be incorrect in the context of the new 
refactor, i.e. with a common entry point to all pin_user_pages() 
exceptions; it was correct originally, with dedicated entry points, as 
all nr_pinned_* were used only after being assigned.


Basically, you could keep everything as it was and just do the 
substitution suggested in (1) and the patch would be as good.


Regards,

Martin


On 6.11.22 г. 17:47 ч., Dawei Li wrote:
> This patch includes changes below:
> 1) pin_user_pages() is unsafe without protection of mmap_lock,
>     fix it by calling mmap_read_lock() & mmap_read_unlock().
> 2) fix & refactor the incorrect exception handling procedure in
>     vmw_mksstat_add_ioctl().
>
> Fixes: 7a7a933edd6c ("drm/vmwgfx: Introduce VMware mks-guest-stats")
> Signed-off-by: Dawei Li <set_pte_at@...look.com>
> ---
> v1:
> https://lore.kernel.org/all/TYCP286MB23235C9A9FCF85C045F95EA7CA4F9@TYCP286MB2323.JPNP286.PROD.OUTLOOK.COM/
>
> v1->v2:
> Rebased to latest vmwgfx/drm-misc-fixes
> ---
>   drivers/gpu/drm/vmwgfx/vmwgfx_msg.c | 23 ++++++++++++++---------
>   1 file changed, 14 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_msg.c b/drivers/gpu/drm/vmwgfx/vmwgfx_msg.c
> index 089046fa21be..ec40a3364e0a 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_msg.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_msg.c
> @@ -1020,9 +1020,9 @@ int vmw_mksstat_add_ioctl(struct drm_device *dev, void *data,
>   	const size_t num_pages_info = PFN_UP(arg->info_len);
>   	const size_t num_pages_strs = PFN_UP(arg->strs_len);
>   	long desc_len;
> -	long nr_pinned_stat;
> -	long nr_pinned_info;
> -	long nr_pinned_strs;
> +	long nr_pinned_stat = 0;
> +	long nr_pinned_info = 0;
> +	long nr_pinned_strs = 0;
>   	struct page *pages_stat[ARRAY_SIZE(pdesc->statPPNs)];
>   	struct page *pages_info[ARRAY_SIZE(pdesc->infoPPNs)];
>   	struct page *pages_strs[ARRAY_SIZE(pdesc->strsPPNs)];
> @@ -1084,28 +1084,33 @@ int vmw_mksstat_add_ioctl(struct drm_device *dev, void *data,
>   	reset_ppn_array(pdesc->infoPPNs, ARRAY_SIZE(pdesc->infoPPNs));
>   	reset_ppn_array(pdesc->strsPPNs, ARRAY_SIZE(pdesc->strsPPNs));
>   
> +	/* pin_user_pages() needs protection of mmap_lock */
> +	mmap_read_lock(current->mm);
> +
>   	/* Pin mksGuestStat user pages and store those in the instance descriptor */
>   	nr_pinned_stat = pin_user_pages(arg->stat, num_pages_stat, FOLL_LONGTERM, pages_stat, NULL);
>   	if (num_pages_stat != nr_pinned_stat)
> -		goto err_pin_stat;
> +		goto __err_pin_pages;
>   
>   	for (i = 0; i < num_pages_stat; ++i)
>   		pdesc->statPPNs[i] = page_to_pfn(pages_stat[i]);
>   
>   	nr_pinned_info = pin_user_pages(arg->info, num_pages_info, FOLL_LONGTERM, pages_info, NULL);
>   	if (num_pages_info != nr_pinned_info)
> -		goto err_pin_info;
> +		goto __err_pin_pages;
>   
>   	for (i = 0; i < num_pages_info; ++i)
>   		pdesc->infoPPNs[i] = page_to_pfn(pages_info[i]);
>   
>   	nr_pinned_strs = pin_user_pages(arg->strs, num_pages_strs, FOLL_LONGTERM, pages_strs, NULL);
>   	if (num_pages_strs != nr_pinned_strs)
> -		goto err_pin_strs;
> +		goto __err_pin_pages;
>   
>   	for (i = 0; i < num_pages_strs; ++i)
>   		pdesc->strsPPNs[i] = page_to_pfn(pages_strs[i]);
>   
> +	mmap_read_unlock(current->mm);
> +
>   	/* Send the descriptor to the host via a hypervisor call. The mksGuestStat
>   	   pages will remain in use until the user requests a matching remove stats
>   	   or a stats reset occurs. */
> @@ -1120,15 +1125,15 @@ int vmw_mksstat_add_ioctl(struct drm_device *dev, void *data,
>   
>   	return 0;
>   
> -err_pin_strs:
> +__err_pin_pages:
> +	mmap_read_unlock(current->mm);
> +
>   	if (nr_pinned_strs > 0)
>   		unpin_user_pages(pages_strs, nr_pinned_strs);
>   
> -err_pin_info:
>   	if (nr_pinned_info > 0)
>   		unpin_user_pages(pages_info, nr_pinned_info);
>   
> -err_pin_stat:
>   	if (nr_pinned_stat > 0)
>   		unpin_user_pages(pages_stat, nr_pinned_stat);
>   

Powered by blists - more mailing lists