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;
From:   ebiederm@xmission.com (Eric W. Biederman)
To:     Andy Lutomirski <luto@kernel.org>
Cc:     x86@kernel.org, LKML <linux-kernel@vger.kernel.org>,
        Andrea Arcangeli <aarcange@redhat.com>,
        Linux-MM <linux-mm@kvack.org>, Jason Gunthorpe <jgg@ziepe.ca>,
        Linus Torvalds <torvalds@linux-foundation.org>,
        Matthew Wilcox <willy@infradead.org>,
        Jann Horn <jannh@google.com>, Jan Kara <jack@suse.cz>,
        Yu Zhao <yuzhao@google.com>, Peter Xu <peterx@redhat.com>
References: <3d34069ab2d249d866ea1d18a47e4170dbfb5982.1610132102.git.luto@kernel.org>
Date:   Sat, 09 Jan 2021 14:16:34 -0600
In-Reply-To: <3d34069ab2d249d866ea1d18a47e4170dbfb5982.1610132102.git.luto@kernel.org>
        (Andy Lutomirski's message of "Fri, 8 Jan 2021 10:55:05 -0800")
Message-ID: <87wnwl27dp.fsf@x220.int.ebiederm.org>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain
Subject: Re: [PATCH] x86/vm86/32: Remove VM86_SCREEN_BITMAP support
Precedence: bulk

Andy Lutomirski <luto@kernel.org> writes:

> The implementation was rather buggy.  It unconditionally marked PTEs
> read-only, even for VM_SHARED mappings.  I'm not sure whether this is
> actually a problem, but it certainly seems unwise.  More importantly, it
> released the mmap lock before flushing the TLB, which could allow a racing
> CoW operation to falsely believe that the underlying memory was not
> writable.
>
> I can't find any users at all of this mechanism, so just remove it.

In another age this was used by dosemu.  Have you looked at dosemu to
see if it still uses this support (on 32bit where dosemu can use vm86)?

It may still be a valid removal target I just wanted to point out what
the original user was.

Eric

> Cc: Andrea Arcangeli <aarcange@redhat.com>
> Cc: Linux-MM <linux-mm@kvack.org>
> Cc: Jason Gunthorpe <jgg@ziepe.ca>
> Cc: x86@kernel.org
> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> Cc: Matthew Wilcox <willy@infradead.org>
> Cc: Jann Horn <jannh@google.com>
> Cc: Jan Kara <jack@suse.cz>
> Cc: Yu Zhao <yuzhao@google.com>
> Cc: Peter Xu <peterx@redhat.com>
> Signed-off-by: Andy Lutomirski <luto@kernel.org>
> ---
>  arch/x86/include/uapi/asm/vm86.h |  2 +-
>  arch/x86/kernel/vm86_32.c        | 55 ++++++--------------------------
>  2 files changed, 10 insertions(+), 47 deletions(-)
>
> diff --git a/arch/x86/include/uapi/asm/vm86.h b/arch/x86/include/uapi/asm/vm86.h
> index d2ee4e307ef8..50004fb4590d 100644
> --- a/arch/x86/include/uapi/asm/vm86.h
> +++ b/arch/x86/include/uapi/asm/vm86.h
> @@ -106,7 +106,7 @@ struct vm86_struct {
>  /*
>   * flags masks
>   */
> -#define VM86_SCREEN_BITMAP	0x0001
> +#define VM86_SCREEN_BITMAP	0x0001        /* no longer supported */
>  
>  struct vm86plus_info_struct {
>  	unsigned long force_return_for_pic:1;
> diff --git a/arch/x86/kernel/vm86_32.c b/arch/x86/kernel/vm86_32.c
> index 764573de3996..28b9e8d511e1 100644
> --- a/arch/x86/kernel/vm86_32.c
> +++ b/arch/x86/kernel/vm86_32.c
> @@ -160,49 +160,6 @@ void save_v86_state(struct kernel_vm86_regs *regs, int retval)
>  	do_exit(SIGSEGV);
>  }
>  
> -static void mark_screen_rdonly(struct mm_struct *mm)
> -{
> -	struct vm_area_struct *vma;
> -	spinlock_t *ptl;
> -	pgd_t *pgd;
> -	p4d_t *p4d;
> -	pud_t *pud;
> -	pmd_t *pmd;
> -	pte_t *pte;
> -	int i;
> -
> -	mmap_write_lock(mm);
> -	pgd = pgd_offset(mm, 0xA0000);
> -	if (pgd_none_or_clear_bad(pgd))
> -		goto out;
> -	p4d = p4d_offset(pgd, 0xA0000);
> -	if (p4d_none_or_clear_bad(p4d))
> -		goto out;
> -	pud = pud_offset(p4d, 0xA0000);
> -	if (pud_none_or_clear_bad(pud))
> -		goto out;
> -	pmd = pmd_offset(pud, 0xA0000);
> -
> -	if (pmd_trans_huge(*pmd)) {
> -		vma = find_vma(mm, 0xA0000);
> -		split_huge_pmd(vma, pmd, 0xA0000);
> -	}
> -	if (pmd_none_or_clear_bad(pmd))
> -		goto out;
> -	pte = pte_offset_map_lock(mm, pmd, 0xA0000, &ptl);
> -	for (i = 0; i < 32; i++) {
> -		if (pte_present(*pte))
> -			set_pte(pte, pte_wrprotect(*pte));
> -		pte++;
> -	}
> -	pte_unmap_unlock(pte, ptl);
> -out:
> -	mmap_write_unlock(mm);
> -	flush_tlb_mm_range(mm, 0xA0000, 0xA0000 + 32*PAGE_SIZE, PAGE_SHIFT, false);
> -}
> -
> -
> -
>  static int do_vm86_irq_handling(int subfunction, int irqnumber);
>  static long do_sys_vm86(struct vm86plus_struct __user *user_vm86, bool plus);
>  
> @@ -282,6 +239,15 @@ static long do_sys_vm86(struct vm86plus_struct __user *user_vm86, bool plus)
>  			offsetof(struct vm86_struct, int_revectored)))
>  		return -EFAULT;
>  
> +
> +	/* VM86_SCREEN_BITMAP had numerous bugs and appears to have no users. */
> +	if (v.flags & VM86_SCREEN_BITMAP) {
> +		char comm[TASK_COMM_LEN];
> +
> +		pr_info_once("vm86: '%s' uses VM86_SCREEN_BITMAP, which is no longer supported\n", get_task_comm(comm, current);
> +		return -EINVAL;
> +	}
> +
>  	memset(&vm86regs, 0, sizeof(vm86regs));
>  
>  	vm86regs.pt.bx = v.regs.ebx;
> @@ -370,9 +336,6 @@ static long do_sys_vm86(struct vm86plus_struct __user *user_vm86, bool plus)
>  	update_task_stack(tsk);
>  	preempt_enable();
>  
> -	if (vm86->flags & VM86_SCREEN_BITMAP)
> -		mark_screen_rdonly(tsk->mm);
> -
>  	memcpy((struct kernel_vm86_regs *)regs, &vm86regs, sizeof(vm86regs));
>  	return regs->ax;
>  }