xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
From: Julien Grall <julien.grall.oss@gmail.com>
To: Stefano Stabellini <sstabellini@kernel.org>
Cc: xen-devel <xen-devel@lists.xenproject.org>,
	Wei Chen <Wei.Chen@arm.com>,
	Henry.Wang@arm.com,  Penny.Zheng@arm.com,
	Bertrand Marquis <Bertrand.Marquis@arm.com>,
	 Julien Grall <jgrall@amazon.com>,
	Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
Subject: Re: [PATCH RFCv2 08/15] xen/arm32: mm: Check if the virtual address is shared before updating it
Date: Thu, 13 May 2021 23:59:13 +0100	[thread overview]
Message-ID: <CAJ=z9a18zq06AKTGRJHHzR1JeabdO+-FKTmu77ZmdPdQQi=NMA@mail.gmail.com> (raw)
In-Reply-To: <alpine.DEB.2.21.2105131528230.5018@sstabellini-ThinkPad-T480s>

[-- Attachment #1: Type: text/plain, Size: 4107 bytes --]

On Thu, 13 May 2021, 23:32 Stefano Stabellini, <sstabellini@kernel.org>
wrote:

> On Wed, 12 May 2021, Julien Grall wrote:
> > Hi Stefano,
> >
> > On 12/05/2021 23:00, Stefano Stabellini wrote:
> > > On Sun, 25 Apr 2021, Julien Grall wrote:
> > > > From: Julien Grall <jgrall@amazon.com>
> > > >
> > > > Only the first 2GB of the virtual address space is shared between all
> > > > the page-tables on Arm32.
> > > >
> > > > There is a long outstanding TODO in xen_pt_update() stating that the
> > > > function is can only work with shared mapping. Nobody has ever called
> > >             ^ remove
> > >
> > > > the function with private mapping, however as we add more callers
> > > > there is a risk to mess things up.
> > > >
> > > > Introduce a new define to mark the ened of the shared mappings and
> use
> > >                                       ^end
> > >
> > > > it in xen_pt_update() to verify if the address is correct.
> > > >
> > > > Note that on Arm64, all the mappings are shared. Some compiler may
> > > > complain about an always true check, so the new define is not
> introduced
> > > > for arm64 and the code is protected with an #ifdef.
> > >   On arm64 we could maybe define SHARED_VIRT_END to an arbitrarely
> large
> > > value, such as:
> > >
> > > #define SHARED_VIRT_END (1UL<<48)
> > >
> > > or:
> > >
> > > #define SHARED_VIRT_END DIRECTMAP_VIRT_END
> > >
> > > ?
> >
> > I thought about it but I didn't want to define to a random value... I
> felt not
> > define it was better.
>
> Yeah, I see your point: any restrictions in addressing (e.g. 48bits)
> are physical address restrictions. Here we are talking about virtual
> address restriction, and I don't think there are actually any
> restrictions there?  We could validly map something at
> 0xffff_ffff_ffff_ffff. So even (1<<48) which makes sense at the physical
> level, doesn't make sense in terms of virtual addresses.
>

The limit for the virtual address is 2^64.


>
> > > > Signed-off-by: Julien Grall <jgrall@amazon.com>
> > > >
> > > > ---
> > > >      Changes in v2:
> > > >          - New patch
> > > > ---
> > > >   xen/arch/arm/mm.c            | 11 +++++++++--
> > > >   xen/include/asm-arm/config.h |  4 ++++
> > > >   2 files changed, 13 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> > > > index 8fac24d80086..5c17cafff847 100644
> > > > --- a/xen/arch/arm/mm.c
> > > > +++ b/xen/arch/arm/mm.c
> > > > @@ -1275,11 +1275,18 @@ static int xen_pt_update(unsigned long virt,
> > > >        * For arm32, page-tables are different on each CPUs. Yet, they
> > > > share
> > > >        * some common mappings. It is assumed that only common
> mappings
> > > >        * will be modified with this function.
> > > > -     *
> > > > -     * XXX: Add a check.
> > > >        */
> > > >       const mfn_t root = virt_to_mfn(THIS_CPU_PGTABLE);
> > > >   +#ifdef SHARED_VIRT_END
> > > > +    if ( virt > SHARED_VIRT_END ||
> > > > +         (SHARED_VIRT_END - virt) < nr_mfns )
> > >
> > > The following would be sufficient, right?
> > >
> > >      if ( virt + nr_mfns > SHARED_VIRT_END )
> >
> > This would not protect against an overflow. So I think it is best if we
> keep
> > my version.
>
> But there can be no overflow with the way SHARED_VIRT_END is defined.

Even if SHARED_VIRT_END was defined at (1<<48) there can be no overflow.
> Only if we defined SHARED_VIRT_END as 0xffff_ffff_ffff_ffff we would
> have an overflow, but you wrote above that your preference is not to do
> that.
>

You can have an overflow regardless the value of SHARED_VIRT_END.

Imagine virt = 2^64 - 1 and nr_mfs = 1. The addition would result to 0.

As a consequence the check would pass when it should not.

One can argue that the caller will always provide sane values. However
given the simplicity of the check, this is not worth the trouble if a
caller is buggy.

Now, the problem with SHARED_VIRT_END equals to 2^64 - 1 is not the
overflow but the compiler that may throw an error/warning for always true
check. Hence the reason of not defining SHARED_VIRT_END on arm64.

Cheers,

[-- Attachment #2: Type: text/html, Size: 6329 bytes --]

  reply	other threads:[~2021-05-13 22:59 UTC|newest]

Thread overview: 59+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-25 20:13 [PATCH RFCv2 00/15] xen/arm: mm: Remove open-coding mappings Julien Grall
2021-04-25 20:13 ` [PATCH RFCv2 01/15] xen/arm: lpae: Rename LPAE_ENTRIES_MASK_GS to LPAE_ENTRY_MASK_GS Julien Grall
2021-05-11 22:12   ` Stefano Stabellini
2021-04-25 20:13 ` [PATCH RFCv2 02/15] xen/arm: lpae: Use the generic helpers to defined the Xen PT helpers Julien Grall
2021-05-11 22:26   ` Stefano Stabellini
2021-05-12 14:26     ` Julien Grall
2021-05-12 21:30       ` Stefano Stabellini
2021-05-12 22:16         ` Julien Grall
2021-05-13 22:44           ` Stefano Stabellini
2021-07-03 17:32             ` Julien Grall
2021-07-13 20:53               ` Stefano Stabellini
2021-07-14 17:40                 ` Julien Grall
2021-04-25 20:13 ` [PATCH RFCv2 03/15] xen/arm: p2m: Replace level_{orders, masks} arrays with LEVEL_{ORDER, MASK} Julien Grall
2021-05-11 22:33   ` Stefano Stabellini
2021-05-12 14:39     ` Julien Grall
2021-04-25 20:13 ` [PATCH RFCv2 04/15] xen/arm: mm: Allow other mapping size in xen_pt_update_entry() Julien Grall
2021-05-11 22:42   ` Stefano Stabellini
2021-04-25 20:13 ` [PATCH RFCv2 05/15] xen/arm: mm: Avoid flushing the TLBs when mapping are inserted Julien Grall
2021-05-11 22:51   ` Stefano Stabellini
2021-04-25 20:13 ` [PATCH RFCv2 06/15] xen/arm: mm: Don't open-code Xen PT update in remove_early_mappings() Julien Grall
2021-05-11 22:58   ` Stefano Stabellini
2021-04-25 20:13 ` [PATCH RFCv2 07/15] xen/arm: mm: Re-implement early_fdt_map() using map_pages_to_xen() Julien Grall
2021-05-12 21:41   ` Stefano Stabellini
2021-05-12 22:18     ` Julien Grall
2021-04-25 20:13 ` [PATCH RFCv2 08/15] xen/arm32: mm: Check if the virtual address is shared before updating it Julien Grall
2021-05-12 22:00   ` Stefano Stabellini
2021-05-12 22:23     ` Julien Grall
2021-05-13 22:32       ` Stefano Stabellini
2021-05-13 22:59         ` Julien Grall [this message]
2021-05-14  1:04           ` Stefano Stabellini
2021-04-25 20:13 ` [PATCH RFCv2 09/15] xen/arm32: mm: Re-implement setup_xenheap_mappings() using map_pages_to_xen() Julien Grall
2021-05-12 22:07   ` Stefano Stabellini
2021-05-13 17:55     ` Julien Grall
2021-04-25 20:13 ` [PATCH RFCv2 10/15] xen/arm: mm: Allocate xen page tables in domheap rather than xenheap Julien Grall
2021-05-12 22:44   ` Stefano Stabellini
2021-05-13 18:09     ` Julien Grall
2021-05-13 22:27       ` Stefano Stabellini
2021-05-15  8:48         ` Julien Grall
2021-05-18  0:37           ` Stefano Stabellini
2021-04-25 20:13 ` [PATCH RFCv2 11/15] xen/arm: mm: Allow page-table allocation from the boot allocator Julien Grall
2021-05-13 22:58   ` Stefano Stabellini
2021-04-25 20:13 ` [PATCH RFCv2 12/15] xen/arm: add Persistent Map (PMAP) infrastructure Julien Grall
2021-04-26  9:41   ` Xia, Hongyan
2021-07-03 17:57     ` Julien Grall
2021-04-28 21:47   ` Wei Liu
2021-05-14 23:25   ` Stefano Stabellini
2021-05-15  8:54     ` Julien Grall
2021-04-25 20:13 ` [PATCH RFCv2 13/15] xen/arm: mm: Use the PMAP helpers in xen_{,un}map_table() Julien Grall
2021-05-14 23:35   ` Stefano Stabellini
2021-05-15  9:03     ` Julien Grall
2021-04-25 20:13 ` [PATCH RFCv2 14/15] xen/arm: mm: Rework setup_xenheap_mappings() Julien Grall
2021-05-14 23:51   ` Stefano Stabellini
2021-05-15  9:21     ` Julien Grall
2021-05-18  0:50       ` Stefano Stabellini
2022-02-12 19:16         ` Julien Grall
2021-04-25 20:13 ` [PATCH RFCv2 15/15] xen/arm: mm: Re-implement setup_frame_table_mappings() with map_pages_to_xen() Julien Grall
2021-05-15  0:02   ` Stefano Stabellini
2021-05-15  9:25     ` Julien Grall
2021-05-18  0:51       ` Stefano Stabellini

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='CAJ=z9a18zq06AKTGRJHHzR1JeabdO+-FKTmu77ZmdPdQQi=NMA@mail.gmail.com' \
    --to=julien.grall.oss@gmail.com \
    --cc=Bertrand.Marquis@arm.com \
    --cc=Henry.Wang@arm.com \
    --cc=Penny.Zheng@arm.com \
    --cc=Volodymyr_Babchuk@epam.com \
    --cc=Wei.Chen@arm.com \
    --cc=jgrall@amazon.com \
    --cc=sstabellini@kernel.org \
    --cc=xen-devel@lists.xenproject.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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).