diff mbox series

[1/3] mm/pagemap: Fix ioctl(PAGEMAP_SCAN) on vma check

Message ID 20231116201547.536857-2-peterx@redhat.com
State New
Headers
Series mm/pagemap: A few fixes to the recent PAGEMAP_SCAN |

Commit Message

Peter Xu Nov. 16, 2023, 8:15 p.m. UTC 
  The new ioctl(PAGEMAP_SCAN) relies on vma wr-protect capability provided by
userfault, however in the vma test it didn't explicitly require the vma to
have wr-protect function enabled, even if PM_SCAN_WP_MATCHING flag is set.

It means the pagemap code can now apply uffd-wp bit to a page in the vma
even if not registered to userfaultfd at all.

Then in whatever way as long as the pte got written and page fault
resolved, we'll apply the write bit even if uffd-wp bit is set.  We'll see
a pte that has both UFFD_WP and WRITE bit set.  Anything later that looks
up the pte for uffd-wp bit will trigger the warning:

WARNING: CPU: 1 PID: 5071 at arch/x86/include/asm/pgtable.h:403 pte_uffd_wp arch/x86/include/asm/pgtable.h:403 [inline]

Fix it by doing proper check over the vma attributes when
PM_SCAN_WP_MATCHING is specified.

Fixes: 52526ca7fdb9 ("fs/proc/task_mmu: implement IOCTL to get and optionally clear info about PTEs")
Reported-by: syzbot+e94c5aaf7890901ebf9b@syzkaller.appspotmail.com
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 fs/proc/task_mmu.c | 24 ++++++++++++++++++++----
 1 file changed, 20 insertions(+), 4 deletions(-)

Comments

David Hildenbrand Nov. 16, 2023, 11:10 p.m. UTC | #1 
On 16.11.23 21:15, Peter Xu wrote:
> The new ioctl(PAGEMAP_SCAN) relies on vma wr-protect capability provided by
> userfault, however in the vma test it didn't explicitly require the vma to
> have wr-protect function enabled, even if PM_SCAN_WP_MATCHING flag is set.
> 
> It means the pagemap code can now apply uffd-wp bit to a page in the vma
> even if not registered to userfaultfd at all.
> 
> Then in whatever way as long as the pte got written and page fault
> resolved, we'll apply the write bit even if uffd-wp bit is set.  We'll see
> a pte that has both UFFD_WP and WRITE bit set.  Anything later that looks
> up the pte for uffd-wp bit will trigger the warning:
> 
> WARNING: CPU: 1 PID: 5071 at arch/x86/include/asm/pgtable.h:403 pte_uffd_wp arch/x86/include/asm/pgtable.h:403 [inline]
> 
> Fix it by doing proper check over the vma attributes when
> PM_SCAN_WP_MATCHING is specified.
> 
> Fixes: 52526ca7fdb9 ("fs/proc/task_mmu: implement IOCTL to get and optionally clear info about PTEs")
> Reported-by: syzbot+e94c5aaf7890901ebf9b@syzkaller.appspotmail.com
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---

Reviewed-by: David Hildenbrand <david@redhat.com>
Andrei Vagin Nov. 17, 2023, 3:26 p.m. UTC | #2 
On Thu, Nov 16, 2023 at 12:15 PM Peter Xu <peterx@redhat.com> wrote:
>
> The new ioctl(PAGEMAP_SCAN) relies on vma wr-protect capability provided by
> userfault, however in the vma test it didn't explicitly require the vma to
> have wr-protect function enabled, even if PM_SCAN_WP_MATCHING flag is set.
>
> It means the pagemap code can now apply uffd-wp bit to a page in the vma
> even if not registered to userfaultfd at all.
>
> Then in whatever way as long as the pte got written and page fault
> resolved, we'll apply the write bit even if uffd-wp bit is set.  We'll see
> a pte that has both UFFD_WP and WRITE bit set.  Anything later that looks
> up the pte for uffd-wp bit will trigger the warning:
>
> WARNING: CPU: 1 PID: 5071 at arch/x86/include/asm/pgtable.h:403 pte_uffd_wp arch/x86/include/asm/pgtable.h:403 [inline]
>
> Fix it by doing proper check over the vma attributes when
> PM_SCAN_WP_MATCHING is specified.
>
> Fixes: 52526ca7fdb9 ("fs/proc/task_mmu: implement IOCTL to get and optionally clear info about PTEs")
> Reported-by: syzbot+e94c5aaf7890901ebf9b@syzkaller.appspotmail.com
> Signed-off-by: Peter Xu <peterx@redhat.com>

Reviewed-by: Andrei Vagin <avagin@gmail.com>

> ---
>  fs/proc/task_mmu.c | 24 ++++++++++++++++++++----
>  1 file changed, 20 insertions(+), 4 deletions(-)
>
> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> index 51e0ec658457..e91085d79926 100644
> --- a/fs/proc/task_mmu.c
> +++ b/fs/proc/task_mmu.c
> @@ -1994,15 +1994,31 @@ static int pagemap_scan_test_walk(unsigned long start, unsigned long end,
>         struct pagemap_scan_private *p = walk->private;
>         struct vm_area_struct *vma = walk->vma;
>         unsigned long vma_category = 0;
> +       bool wp_allowed = userfaultfd_wp_async(vma) &&
> +           userfaultfd_wp_use_markers(vma);
>
> -       if (userfaultfd_wp_async(vma) && userfaultfd_wp_use_markers(vma))
> -               vma_category |= PAGE_IS_WPALLOWED;
> -       else if (p->arg.flags & PM_SCAN_CHECK_WPASYNC)
> -               return -EPERM;
> +       if (!wp_allowed) {
> +               /* User requested explicit failure over wp-async capability */
> +               if (p->arg.flags & PM_SCAN_CHECK_WPASYNC)
> +                       return -EPERM;
> +               /*
> +                * User requires wr-protect, and allows silently skipping
> +                * unsupported vmas.
> +                */
> +               if (p->arg.flags & PM_SCAN_WP_MATCHING)
> +                       return 1;
> +               /*
> +                * Then the request doesn't involve wr-protects at all,
> +                * fall through to the rest checks, and allow vma walk.
> +                */
> +       }
>
>         if (vma->vm_flags & VM_PFNMAP)
>                 return 1;
>
> +       if (wp_allowed)
> +               vma_category |= PAGE_IS_WPALLOWED;
> +
>         if (vma->vm_flags & VM_SOFTDIRTY)
>                 vma_category |= PAGE_IS_SOFT_DIRTY;
>
> --
> 2.41.0
>
Muhammad Usama Anjum Nov. 19, 2023, 3:54 p.m. UTC | #3 
Hi Peter,

Thank you for taking care of it. I'm on holidays after LPC.

On 11/16/23 3:15 PM, Peter Xu wrote:
> The new ioctl(PAGEMAP_SCAN) relies on vma wr-protect capability provided by
> userfault, however in the vma test it didn't explicitly require the vma to
> have wr-protect function enabled, even if PM_SCAN_WP_MATCHING flag is set.
> 
> It means the pagemap code can now apply uffd-wp bit to a page in the vma
> even if not registered to userfaultfd at all.
> 
> Then in whatever way as long as the pte got written and page fault
> resolved, we'll apply the write bit even if uffd-wp bit is set.  We'll see
> a pte that has both UFFD_WP and WRITE bit set.  Anything later that looks
> up the pte for uffd-wp bit will trigger the warning:
> 
> WARNING: CPU: 1 PID: 5071 at arch/x86/include/asm/pgtable.h:403 pte_uffd_wp arch/x86/include/asm/pgtable.h:403 [inline]
> 
> Fix it by doing proper check over the vma attributes when
> PM_SCAN_WP_MATCHING is specified.
> 
> Fixes: 52526ca7fdb9 ("fs/proc/task_mmu: implement IOCTL to get and optionally clear info about PTEs")
> Reported-by: syzbot+e94c5aaf7890901ebf9b@syzkaller.appspotmail.com
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  fs/proc/task_mmu.c | 24 ++++++++++++++++++++----
>  1 file changed, 20 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> index 51e0ec658457..e91085d79926 100644
> --- a/fs/proc/task_mmu.c
> +++ b/fs/proc/task_mmu.c
> @@ -1994,15 +1994,31 @@ static int pagemap_scan_test_walk(unsigned long start, unsigned long end,
>  	struct pagemap_scan_private *p = walk->private;
>  	struct vm_area_struct *vma = walk->vma;
>  	unsigned long vma_category = 0;
> +	bool wp_allowed = userfaultfd_wp_async(vma) &&
> +	    userfaultfd_wp_use_markers(vma);
>  
> -	if (userfaultfd_wp_async(vma) && userfaultfd_wp_use_markers(vma))
> -		vma_category |= PAGE_IS_WPALLOWED;
> -	else if (p->arg.flags & PM_SCAN_CHECK_WPASYNC)
> -		return -EPERM;
> +	if (!wp_allowed) {
> +		/* User requested explicit failure over wp-async capability */
> +		if (p->arg.flags & PM_SCAN_CHECK_WPASYNC)
> +			return -EPERM;
> +		/*
> +		 * User requires wr-protect, and allows silently skipping
> +		 * unsupported vmas.
> +		 */
> +		if (p->arg.flags & PM_SCAN_WP_MATCHING)
> +			return 1;
> +		/*
> +		 * Then the request doesn't involve wr-protects at all,
> +		 * fall through to the rest checks, and allow vma walk.
> +		 */
> +	}
Very simply done. I've really liked it.

Reviewed-by: Muhammad Usama Anjum <usama.anjum@collabora.com>

>  
>  	if (vma->vm_flags & VM_PFNMAP)
>  		return 1;
>  
> +	if (wp_allowed)
> +		vma_category |= PAGE_IS_WPALLOWED;
> +
>  	if (vma->vm_flags & VM_SOFTDIRTY)
>  		vma_category |= PAGE_IS_SOFT_DIRTY;
>
diff mbox series

Patch

diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index 51e0ec658457..e91085d79926 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -1994,15 +1994,31 @@  static int pagemap_scan_test_walk(unsigned long start, unsigned long end,
 	struct pagemap_scan_private *p = walk->private;
 	struct vm_area_struct *vma = walk->vma;
 	unsigned long vma_category = 0;
+	bool wp_allowed = userfaultfd_wp_async(vma) &&
+	    userfaultfd_wp_use_markers(vma);
 
-	if (userfaultfd_wp_async(vma) && userfaultfd_wp_use_markers(vma))
-		vma_category |= PAGE_IS_WPALLOWED;
-	else if (p->arg.flags & PM_SCAN_CHECK_WPASYNC)
-		return -EPERM;
+	if (!wp_allowed) {
+		/* User requested explicit failure over wp-async capability */
+		if (p->arg.flags & PM_SCAN_CHECK_WPASYNC)
+			return -EPERM;
+		/*
+		 * User requires wr-protect, and allows silently skipping
+		 * unsupported vmas.
+		 */
+		if (p->arg.flags & PM_SCAN_WP_MATCHING)
+			return 1;
+		/*
+		 * Then the request doesn't involve wr-protects at all,
+		 * fall through to the rest checks, and allow vma walk.
+		 */
+	}
 
 	if (vma->vm_flags & VM_PFNMAP)
 		return 1;
 
+	if (wp_allowed)
+		vma_category |= PAGE_IS_WPALLOWED;
+
 	if (vma->vm_flags & VM_SOFTDIRTY)
 		vma_category |= PAGE_IS_SOFT_DIRTY;