From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-9.8 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,HTML_MESSAGE,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 22C9BC433B4 for ; Thu, 13 May 2021 22:59:35 +0000 (UTC) Received: from lists.xenproject.org (lists.xenproject.org [192.237.175.120]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id B84FA61363 for ; Thu, 13 May 2021 22:59:34 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org B84FA61363 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=gmail.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=xen-devel-bounces@lists.xenproject.org Received: from list by lists.xenproject.org with outflank-mailman.127106.238810 (Exim 4.92) (envelope-from ) id 1lhKIh-0001gk-4Y; Thu, 13 May 2021 22:59:27 +0000 X-Outflank-Mailman: Message body and most headers restored to incoming version Received: by outflank-mailman (output) from mailman id 127106.238810; Thu, 13 May 2021 22:59:27 +0000 Received: from localhost ([127.0.0.1] helo=lists.xenproject.org) by lists.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1lhKIh-0001gd-1X; Thu, 13 May 2021 22:59:27 +0000 Received: by outflank-mailman (input) for mailman id 127106; Thu, 13 May 2021 22:59:25 +0000 Received: from us1-rack-iad1.inumbo.com ([172.99.69.81]) by lists.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1lhKIf-0001gT-Ns for xen-devel@lists.xenproject.org; Thu, 13 May 2021 22:59:25 +0000 Received: from mail-ej1-x632.google.com (unknown [2a00:1450:4864:20::632]) by us1-rack-iad1.inumbo.com (Halon) with ESMTPS id 9aef104c-0bfb-4de3-a30f-f3b27be6dfd3; Thu, 13 May 2021 22:59:24 +0000 (UTC) Received: by mail-ej1-x632.google.com with SMTP id u21so42028182ejo.13 for ; Thu, 13 May 2021 15:59:24 -0700 (PDT) X-BeenThere: xen-devel@lists.xenproject.org List-Id: Xen developer discussion List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xenproject.org Precedence: list Sender: "Xen-devel" X-Inumbo-ID: 9aef104c-0bfb-4de3-a30f-f3b27be6dfd3 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=iHbZSskysz//rzg/J/U/CPVDeQUSyKDFXBsEtO6wAsk=; b=c4IiTnwj5ZjXKB15fI1gV8Jew6IFfOEirnZKZGW+PQf7m9+u+zYsfzsTmNKOzcYCf2 TufKQQjfu+4uu3JQ4SB+eLT1FOWlXzUKPXBdIMprlScFAFSTZpD5gToIPRhPzIH/YeIF d0p8v4ykEh3DYmLa6FZ8QKT5nBH/1xCCzoIv/wiOU3SWd7n8/LWLphVLMLT5SuJoFP4U 8s3HYNxGpS36X0ZRPPUVhzizSmzSgP9Tq7Knjby12nf3dI/frygAUc3YE97nd4uHXqBQ MokHJqAMxAVAxTxMY1RiFWZFyxGKw4c/w+tG3vBvXEKkyMG7E9krUpFiD9+wZDaXy1Qy RZSw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=iHbZSskysz//rzg/J/U/CPVDeQUSyKDFXBsEtO6wAsk=; b=eknWbGHbid2m8PJG7AVkX+mUqBCq6X0mL7pgWDCJyvcyfNIy31/9/gqg7yU8LxfbI+ /9JrLM8mF0JqWXBA/CR7r48+/oCxGRDDumgaZY4/UZtU605maD/znY13HRBgiHYQm4pG vq6tttLGAHjziYsFgb8hweFYOyzZ+rEWMV/tiR4ftOpZZ0EONOc5YV4he9AaCRGStLHJ ixrfEkiec4arnPU3H1rHDYmOas91Km6EFViyCqLTfnSsNmRvSCjA+VkTWlbIz9eMuzEp MxAfDg5XJ3LQqHwgJqqOCiuYqXOJXt7RqPGCW6HZa3G+hOok3VXBHwaaa8Q5mpyqQhiz dMbg== X-Gm-Message-State: AOAM532rIC2xVxP2bETgMudovWb4Tltn4lMRpD+JU2Sh0R2Gwbd2+4w9 VwJTf6pS0U8+bBlEuR9RzKqTPj5goa0eIZQyRXk= X-Google-Smtp-Source: ABdhPJxE/CH8XOP455+qI0x7WsL04Do3u1JmWZNPPEWo6czS+atSjxl6pF4Xtowu/DWbujf1MOanWlE3MNZD1HrHr4w= X-Received: by 2002:a17:906:e28c:: with SMTP id gg12mr45600715ejb.483.1620946763640; Thu, 13 May 2021 15:59:23 -0700 (PDT) MIME-Version: 1.0 References: <20210425201318.15447-1-julien@xen.org> <20210425201318.15447-9-julien@xen.org> In-Reply-To: From: Julien Grall Date: Thu, 13 May 2021 23:59:13 +0100 Message-ID: Subject: Re: [PATCH RFCv2 08/15] xen/arm32: mm: Check if the virtual address is shared before updating it To: Stefano Stabellini Cc: xen-devel , Wei Chen , Henry.Wang@arm.com, Penny.Zheng@arm.com, Bertrand Marquis , Julien Grall , Volodymyr Babchuk Content-Type: multipart/alternative; boundary="0000000000000bf2b905c23e143d" --0000000000000bf2b905c23e143d Content-Type: text/plain; charset="UTF-8" On Thu, 13 May 2021, 23:32 Stefano Stabellini, 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 > > > > > > > > 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 > > > > > > > > --- > > > > 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, --0000000000000bf2b905c23e143d Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


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 be= tween 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 ev= er called
> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0^ remove
> >
> > > the function with private mapping, however as we add more ca= llers
> > > there is a risk to mess things up.
> > >
> > > Introduce a new define to mark the ened of the shared mappin= gs and use
> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0^e= nd
> >
> > > it in xen_pt_update() to verify if the address is correct. > > >
> > > Note that on Arm64, all the mappings are shared. Some compil= er may
> > > complain about an always true check, so the new define is no= t introduced
> > > for arm64 and the code is protected with an #ifdef.
> >=C2=A0 =C2=A0On 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?=C2=A0 We could validly map something at
0xffff_ffff_ffff_ffff. So even (1<<48) which makes sense at the physi= cal
level, doesn't make sense in terms of virtual addresses.

The limit for t= he virtual address is 2^64.



> > > Signed-off-by: Julien Grall <jgrall@amazon.com>
> > >
> > > ---
> > >=C2=A0 =C2=A0 =C2=A0 Changes in v2:
> > >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 - New patch
> > > ---
> > >=C2=A0 =C2=A0xen/arch/arm/mm.c=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 | 11 +++++++++--
> > >=C2=A0 =C2=A0xen/include/asm-arm/config.h |=C2=A0 4 ++++
> > >=C2=A0 =C2=A02 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 lo= ng virt,
> > >=C2=A0 =C2=A0 =C2=A0 =C2=A0 * For arm32, page-tables are diff= erent on each CPUs. Yet, they
> > > share
> > >=C2=A0 =C2=A0 =C2=A0 =C2=A0 * some common mappings. It is ass= umed that only common mappings
> > >=C2=A0 =C2=A0 =C2=A0 =C2=A0 * will be modified with this func= tion.
> > > -=C2=A0 =C2=A0 =C2=A0*
> > > -=C2=A0 =C2=A0 =C2=A0* XXX: Add a check.
> > >=C2=A0 =C2=A0 =C2=A0 =C2=A0 */
> > >=C2=A0 =C2=A0 =C2=A0 =C2=A0const mfn_t root =3D virt_to_mfn(T= HIS_CPU_PGTABLE);
> > >=C2=A0 =C2=A0+#ifdef SHARED_VIRT_END
> > > +=C2=A0 =C2=A0 if ( virt > SHARED_VIRT_END ||
> > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0(SHARED_VIRT_END - virt) = < nr_mfns )
> >
> > The following would be sufficient, right?
> >
> >=C2=A0 =C2=A0 =C2=A0 if ( virt + nr_mfns > SHARED_VIRT_END ) >
> This would not protect against an overflow. So I think it is best if w= e 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 overfl= ow.
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 =3D 2^64 - 1 an= d nr_mfs =3D 1. The addition would result to 0.

=
As a consequence the check would pass when it shoul= d not.

One can argue tha= t 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 S= HARED_VIRT_END on arm64.

Cheers,

--0000000000000bf2b905c23e143d--