Re: [PATCH v4 1/7] userfaultfd: move core VMA manipulation logic to mm/userfaultfd.c

[Pengfei Xu <pengfei.xu@intel.com>]

Hi Lorenzo Stoakes,

On 2024-08-07 at 13:03:52 +0100, Lorenzo Stoakes wrote:
> On Wed, Aug 07, 2024 at 11:45:56AM GMT, Pengfei Xu wrote:
> > Hi Lorenzo Stoakes,
> >
> > Greetings!
> >
> > I used syzkaller and found
> > KASAN: slab-use-after-free Read in userfaultfd_set_ctx in next-20240805.
> >
> > Bisected the first bad commit:
> > 4651ba8201cf userfaultfd: move core VMA manipulation logic to mm/userfaultfd.c
> >
> > All detailed info: https://github.com/xupengfe/syzkaller_logs/tree/main/240806_122723_userfaultfd_set_ct
> 
> [snip]
> 
> Andrew - As this is so small, could you take this as a fix-patch? The fix
> is enclosed below.
> 
> 
> Pengfei - Sorry for the delay on getting this resolved, I was struggling to
> repro with my usual dev setup, after trying a lot of things I ended up
> using the supplied repro env and was able to do so there.
> 
> (I suspect that VMAs are laid out slightly differently in my usual arch base
> image perhaps based on tunables, and this was the delta on that!)
> 
> Regardless, I was able to identify the cause - we incorrectly pass a stale
> pointer to userfaultfd_reset_ctx() if a merge is performed in
> userfaultfd_clear_vma().
> 
> This was a subtle mistake on my part, I don't see any other instances like
> this in the patch.
> 
> Syzkaller managed to get this merge to happen and kasan picked up on it, so
> thank you very much for supplying the infra!
> 
> The fix itself is very simple, a one-liner, enclosed below.
> 

Thanks for your patch!
I tested the below patch on top of next-20240805, this issue could not
be reproduced. So it's fixed.

Best Regrads,
Thanks!


> ----8<----
> From 193abd1c3a51e6bf1d85ddfe01845e9713336970 Mon Sep 17 00:00:00 2001
> From: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> Date: Wed, 7 Aug 2024 12:44:27 +0100
> Subject: [PATCH] mm: userfaultfd: fix user-after-free in
>  userfaultfd_clear_vma()
> 
> After invoking vma_modify_flags_uffd() in userfaultfd_clear_vma(), we may
> have merged the vma, and depending on the kind of merge, deleted the vma,
> rendering the vma pointer invalid.
> 
> The code incorrectly referenced this now possibly invalid vma pointer when
> invoking userfaultfd_reset_ctx().
> 
> If no merge is possible, vma_modify_flags_uffd() performs a split and
> returns the original vma. Therefore the correct approach is to simply pass
> the ret pointer to userfaultfd_ret_ctx().
> 
> Reported-by: Pengfei Xu <pengfei.xu@intel.com>
> Fixes: e310f2b78a77 ("userfaultfd: move core VMA manipulation logic to mm/userfaultfd.c")
> Closes: https://lore.kernel.org/all/ZrLt9HIxV9QiZotn@xpf.sh.intel.com/
> Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> ---
>  mm/userfaultfd.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
> index 3b7715ecf292..966e6c81a685 100644
> --- a/mm/userfaultfd.c
> +++ b/mm/userfaultfd.c
> @@ -1813,7 +1813,7 @@ struct vm_area_struct *userfaultfd_clear_vma(struct vma_iterator *vmi,
>  	 * the current one has not been updated yet.
>  	 */
>  	if (!IS_ERR(ret))
> -		userfaultfd_reset_ctx(vma);
> +		userfaultfd_reset_ctx(ret);
> 
>  	return ret;
>  }
> --
> 2.45.2