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

From: Pengfei Xu
Date: Thu Aug 08 2024 - 01:23:58 EST


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@xxxxxxxxxx>
> 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@xxxxxxxxx>
> Fixes: e310f2b78a77 ("userfaultfd: move core VMA manipulation logic to mm/userfaultfd.c")
> Closes: https://lore.kernel.org/all/ZrLt9HIxV9QiZotn@xxxxxxxxxxxxxxxx/
> Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@xxxxxxxxxx>
> ---
> 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