All of lore.kernel.org
 help / color / mirror / Atom feed
From: Dmitry Safonov <dima@arista.com>
To: Michael Ellerman <mpe@ellerman.id.au>,
	Christophe Leroy <christophe.leroy@csgroup.eu>,
	linux-kernel@vger.kernel.org
Cc: Dmitry Safonov <0x7f454c46@gmail.com>,
	Andrei Vagin <avagin@gmail.com>,
	Andy Lutomirski <luto@kernel.org>,
	Benjamin Herrenschmidt <benh@kernel.crashing.org>,
	Laurent Dufour <ldufour@linux.ibm.com>,
	Paul Mackerras <paulus@samba.org>,
	linuxppc-dev@lists.ozlabs.org, stable@vger.kernel.org
Subject: Re: [PATCH] powerpc/vdso: Separate vvar vma from vdso
Date: Wed, 31 Mar 2021 19:53:46 +0100	[thread overview]
Message-ID: <361ec8ba-8335-157a-53e8-38a656626519@arista.com> (raw)
In-Reply-To: <8735wby77v.fsf@mpe.ellerman.id.au>

On 3/31/21 10:59 AM, Michael Ellerman wrote:
> Christophe Leroy <christophe.leroy@csgroup.eu> writes:
[..]
>>
>>> @@ -133,7 +135,13 @@ static int __arch_setup_additional_pages(struct linux_binprm *bprm, int uses_int
>>>   	 * install_special_mapping or the perf counter mmap tracking code
>>>   	 * will fail to recognise it as a vDSO.
>>>   	 */
>>> -	mm->context.vdso = (void __user *)vdso_base + PAGE_SIZE;
>>> +	mm->context.vdso = (void __user *)vdso_base + vvar_size;
>>> +
>>> +	vma = _install_special_mapping(mm, vdso_base, vvar_size,
>>> +				       VM_READ | VM_MAYREAD | VM_IO |
>>> +				       VM_DONTDUMP | VM_PFNMAP, &vvar_spec);
>>> +	if (IS_ERR(vma))
>>> +		return PTR_ERR(vma);
>>>   
>>>   	/*
>>>   	 * our vma flags don't have VM_WRITE so by default, the process isn't
>>
>>
>> IIUC, VM_PFNMAP is for when we have a vvar_fault handler.
>> Allthough we will soon have one for handle TIME_NS, at the moment
>> powerpc doesn't have that handler.
>> Isn't it dangerous to set VM_PFNMAP then ?

I believe, it's fine, special_mapping_fault() does:
:		if (sm->fault)
:			return sm->fault(sm, vmf->vma, vmf);

> Some of the other flags seem odd too.
> eg. VM_IO ? VM_DONTDUMP ?

Yeah, so:
VM_PFNMAP | VM_IO is a protection from remote access on pages. So one
can't access such page with ptrace(), /proc/$pid/mem or
process_vm_write(). Otherwise, it would create COW mapping and the
tracee will stop working with stale vvar.

VM_DONTDUMP restricts the area from coredumping and gdb will also avoid
accessing those[1][2].

I agree that VM_PFNMAP was probably excessive in this patch alone and
rather synchronized code with other architectures, but it makes more
sense now in the new patches set by Christophe:
https://lore.kernel.org/linux-arch/cover.1617209141.git.christophe.leroy@csgroup.eu/


[1] https://lore.kernel.org/lkml/550731AF.6080904@redhat.com/T/
[2] https://sourceware.org/legacy-ml/gdb-patches/2015-03/msg00383.html

Thanks,
          Dmitry

WARNING: multiple messages have this Message-ID (diff)
From: Dmitry Safonov <dima@arista.com>
To: Michael Ellerman <mpe@ellerman.id.au>,
	Christophe Leroy <christophe.leroy@csgroup.eu>,
	linux-kernel@vger.kernel.org
Cc: Dmitry Safonov <0x7f454c46@gmail.com>,
	stable@vger.kernel.org, Andrei Vagin <avagin@gmail.com>,
	Paul Mackerras <paulus@samba.org>,
	Andy Lutomirski <luto@kernel.org>,
	Laurent Dufour <ldufour@linux.ibm.com>,
	linuxppc-dev@lists.ozlabs.org
Subject: Re: [PATCH] powerpc/vdso: Separate vvar vma from vdso
Date: Wed, 31 Mar 2021 19:53:46 +0100	[thread overview]
Message-ID: <361ec8ba-8335-157a-53e8-38a656626519@arista.com> (raw)
In-Reply-To: <8735wby77v.fsf@mpe.ellerman.id.au>

On 3/31/21 10:59 AM, Michael Ellerman wrote:
> Christophe Leroy <christophe.leroy@csgroup.eu> writes:
[..]
>>
>>> @@ -133,7 +135,13 @@ static int __arch_setup_additional_pages(struct linux_binprm *bprm, int uses_int
>>>   	 * install_special_mapping or the perf counter mmap tracking code
>>>   	 * will fail to recognise it as a vDSO.
>>>   	 */
>>> -	mm->context.vdso = (void __user *)vdso_base + PAGE_SIZE;
>>> +	mm->context.vdso = (void __user *)vdso_base + vvar_size;
>>> +
>>> +	vma = _install_special_mapping(mm, vdso_base, vvar_size,
>>> +				       VM_READ | VM_MAYREAD | VM_IO |
>>> +				       VM_DONTDUMP | VM_PFNMAP, &vvar_spec);
>>> +	if (IS_ERR(vma))
>>> +		return PTR_ERR(vma);
>>>   
>>>   	/*
>>>   	 * our vma flags don't have VM_WRITE so by default, the process isn't
>>
>>
>> IIUC, VM_PFNMAP is for when we have a vvar_fault handler.
>> Allthough we will soon have one for handle TIME_NS, at the moment
>> powerpc doesn't have that handler.
>> Isn't it dangerous to set VM_PFNMAP then ?

I believe, it's fine, special_mapping_fault() does:
:		if (sm->fault)
:			return sm->fault(sm, vmf->vma, vmf);

> Some of the other flags seem odd too.
> eg. VM_IO ? VM_DONTDUMP ?

Yeah, so:
VM_PFNMAP | VM_IO is a protection from remote access on pages. So one
can't access such page with ptrace(), /proc/$pid/mem or
process_vm_write(). Otherwise, it would create COW mapping and the
tracee will stop working with stale vvar.

VM_DONTDUMP restricts the area from coredumping and gdb will also avoid
accessing those[1][2].

I agree that VM_PFNMAP was probably excessive in this patch alone and
rather synchronized code with other architectures, but it makes more
sense now in the new patches set by Christophe:
https://lore.kernel.org/linux-arch/cover.1617209141.git.christophe.leroy@csgroup.eu/


[1] https://lore.kernel.org/lkml/550731AF.6080904@redhat.com/T/
[2] https://sourceware.org/legacy-ml/gdb-patches/2015-03/msg00383.html

Thanks,
          Dmitry

  reply	other threads:[~2021-03-31 18:54 UTC|newest]

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-03-26 19:17 [PATCH] powerpc/vdso: Separate vvar vma from vdso Dmitry Safonov
2021-03-26 19:17 ` Dmitry Safonov
2021-03-27 17:19 ` Christophe Leroy
2021-03-27 17:19   ` Christophe Leroy
2021-03-27 17:43   ` Dmitry Safonov
2021-03-27 17:43     ` Dmitry Safonov
2021-03-29  9:51     ` Laurent Dufour
2021-03-29  9:51       ` Laurent Dufour
2021-03-29 15:14 ` Laurent Dufour
2021-03-29 15:14   ` Laurent Dufour
2021-03-29 19:59   ` Dmitry Safonov
2021-03-29 19:59     ` Dmitry Safonov
2021-03-30  8:41 ` Christophe Leroy
2021-03-30  8:41   ` Christophe Leroy
2021-03-31  9:59   ` Michael Ellerman
2021-03-31  9:59     ` Michael Ellerman
2021-03-31 18:53     ` Dmitry Safonov [this message]
2021-03-31 18:53       ` Dmitry Safonov
2021-03-30 10:17 ` Christophe Leroy
2021-03-30 10:17   ` Christophe Leroy
2021-03-31 18:15   ` Dmitry Safonov
2021-03-31 18:15     ` Dmitry Safonov
2021-04-19  3:59 ` Michael Ellerman
2021-04-19  3:59   ` Michael Ellerman

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=361ec8ba-8335-157a-53e8-38a656626519@arista.com \
    --to=dima@arista.com \
    --cc=0x7f454c46@gmail.com \
    --cc=avagin@gmail.com \
    --cc=benh@kernel.crashing.org \
    --cc=christophe.leroy@csgroup.eu \
    --cc=ldufour@linux.ibm.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linuxppc-dev@lists.ozlabs.org \
    --cc=luto@kernel.org \
    --cc=mpe@ellerman.id.au \
    --cc=paulus@samba.org \
    --cc=stable@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.