Xen-Devel Archive on lore.kernel.org
 help / color / Atom feed
* [Xen-devel] [PATCH for-4.13 v3] xen/arm: fix buf size in make_cpus_node
@ 2019-10-08 23:12 Stefano Stabellini
  2019-10-09 14:48 ` Julien Grall
  0 siblings, 1 reply; 3+ messages in thread
From: Stefano Stabellini @ 2019-10-08 23:12 UTC (permalink / raw)
  To: xen-devel
  Cc: jgross, Julien.Grall, sstabellini, Volodymyr_Babchuk, Stefano Stabellini

The size of buf is calculated wrongly: the number is printed as a
hexadecimal number, so we need 8 bytes for 32bit, not 10 bytes.

As a result, it should be sizeof("cpu@") + 8 bytes for a 32-bit number +
1 byte for \0. Total = 13.

mpidr_aff is 64-bit, however, only bits [0-23] are used. Add a check for
that.

Fixes: c81a791d34 (xen/arm: Set 'reg' of cpu node for dom0 to match MPIDR's affinity)
Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
Release-acked-by: Juergen Gross <jgross@suse.com>
---
Changes in v3:
- make sure only [23:0] bits are used in mpidr_aff
- clarify that we only need 32bit for buf writes

Changes in v2:
- patch added
---
 xen/arch/arm/domain_build.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 921b054520..d5ee639548 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -789,7 +789,7 @@ static int __init make_cpus_node(const struct domain *d, void *fdt)
     const void *compatible = NULL;
     u32 len;
     /* Placeholder for cpu@ + a 32-bit number + \0 */
-    char buf[15];
+    char buf[13];
     u32 clock_frequency;
     bool clock_valid;
     uint64_t mpidr_aff;
@@ -847,8 +847,18 @@ static int __init make_cpus_node(const struct domain *d, void *fdt)
          * the MPIDR's affinity bits. We will use AFF0 and AFF1 when
          * constructing the reg value of the guest at the moment, for it
          * is enough for the current max vcpu number.
+         *
+         * We only deal with AFF{0, 1, 2} stored in bits [23:0] at the
+         * moment.
          */
         mpidr_aff = vcpuid_to_vaffinity(cpu);
+        if ( (mpidr_aff & ~GENMASK_ULL(23, 0)) != 0 )
+        {
+            printk(XENLOG_ERR "Unable to handle MPIDR AFFINITY 0x%"PRIx64"\n", 
+                   mpidr_aff);
+            return -EINVAL;
+        }
+
         dt_dprintk("Create cpu@%"PRIx64" (logical CPUID: %d) node\n",
                    mpidr_aff, cpu);
 
-- 
2.17.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [Xen-devel] [PATCH for-4.13 v3] xen/arm: fix buf size in make_cpus_node
  2019-10-08 23:12 [Xen-devel] [PATCH for-4.13 v3] xen/arm: fix buf size in make_cpus_node Stefano Stabellini
@ 2019-10-09 14:48 ` Julien Grall
  2019-10-10  0:40   ` Stefano Stabellini
  0 siblings, 1 reply; 3+ messages in thread
From: Julien Grall @ 2019-10-09 14:48 UTC (permalink / raw)
  To: Stefano Stabellini, xen-devel
  Cc: jgross, Stefano Stabellini, Volodymyr_Babchuk

Hi Stefano,

On 09/10/2019 00:12, Stefano Stabellini wrote:
> The size of buf is calculated wrongly: the number is printed as a
> hexadecimal number, so we need 8 bytes for 32bit, not 10 bytes.
> 
> As a result, it should be sizeof("cpu@") + 8 bytes for a 32-bit number +
> 1 byte for \0. Total = 13.
> 
> mpidr_aff is 64-bit, however, only bits [0-23] are used. Add a check for
> that.

I am not entirely happy with the commit message. There are no real issue with 
the current code (the buffer is big enough) as mpdir_aff can only have [23:0] 
set in the current code.

The patch is only hardening the code and that should be reflected in the commit 
message.

So how about:

xen/arm: domain_build: Harden make_cpus_node()

make_cpus_node() is using a static buffer to generate the FDT node name.

While mpdir_aff is a 64-bit integer, we only ever use the bits [23:0] as only 
AFF{0, 1, 2} are supported for now.

To avoid any potential issue in the future, check that mpdir_aff has only bits 
[23:0] set.

At the same time, take the opportunity to reduce the size of the buffer. Indeed, 
only 8 characters is useful to generate an 32-bit hexadecimal number. So 
sizeof("cpu@") + 8 = 13 characters is sufficient here.

> 
> Fixes: c81a791d34 (xen/arm: Set 'reg' of cpu node for dom0 to match MPIDR's affinity)
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
> Release-acked-by: Juergen Gross <jgross@suse.com>
> ---
> Changes in v3:
> - make sure only [23:0] bits are used in mpidr_aff
> - clarify that we only need 32bit for buf writes
> 
> Changes in v2:
> - patch added
> ---
>   xen/arch/arm/domain_build.c | 12 +++++++++++-
>   1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 921b054520..d5ee639548 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -789,7 +789,7 @@ static int __init make_cpus_node(const struct domain *d, void *fdt)
>       const void *compatible = NULL;
>       u32 len;
>       /* Placeholder for cpu@ + a 32-bit number + \0 */

I think you want to update the comment to say "32-bit hexa number".

> -    char buf[15];
> +    char buf[13];

This is a confusing code to read because above you mention this is a 32-bit 
number, but below you are using PRIx64. It takes a bit of time to figure out 
that mpdir_aff will always have bits above 32-bit zeroed.

I would prefer to use a temporary variable for the register, but I would be 
happy to consider a suitable comment in code.

>       u32 clock_frequency;
>       bool clock_valid;
>       uint64_t mpidr_aff;
> @@ -847,8 +847,18 @@ static int __init make_cpus_node(const struct domain *d, void *fdt)
>            * the MPIDR's affinity bits. We will use AFF0 and AFF1 when
>            * constructing the reg value of the guest at the moment, for it
>            * is enough for the current max vcpu number.
> +         *
> +         * We only deal with AFF{0, 1, 2} stored in bits [23:0] at the
> +         * moment.
>            */
>           mpidr_aff = vcpuid_to_vaffinity(cpu);
> +        if ( (mpidr_aff & ~GENMASK_ULL(23, 0)) != 0 )
> +        {
> +            printk(XENLOG_ERR "Unable to handle MPIDR AFFINITY 0x%"PRIx64"\n",
> +                   mpidr_aff);
> +            return -EINVAL;
> +        }
> +
>           dt_dprintk("Create cpu@%"PRIx64" (logical CPUID: %d) node\n",
>                      mpidr_aff, cpu);
>   
> 

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [Xen-devel] [PATCH for-4.13 v3] xen/arm: fix buf size in make_cpus_node
  2019-10-09 14:48 ` Julien Grall
@ 2019-10-10  0:40   ` Stefano Stabellini
  0 siblings, 0 replies; 3+ messages in thread
From: Stefano Stabellini @ 2019-10-10  0:40 UTC (permalink / raw)
  To: Julien Grall
  Cc: jgross, xen-devel, Stefano Stabellini, Volodymyr_Babchuk,
	Stefano Stabellini

On Wed, 9 Oct 2019, Julien Grall wrote:
> Hi Stefano,
> 
> On 09/10/2019 00:12, Stefano Stabellini wrote:
> > The size of buf is calculated wrongly: the number is printed as a
> > hexadecimal number, so we need 8 bytes for 32bit, not 10 bytes.
> > 
> > As a result, it should be sizeof("cpu@") + 8 bytes for a 32-bit number +
> > 1 byte for \0. Total = 13.
> > 
> > mpidr_aff is 64-bit, however, only bits [0-23] are used. Add a check for
> > that.
> 
> I am not entirely happy with the commit message. There are no real issue with
> the current code (the buffer is big enough) as mpdir_aff can only have [23:0]
> set in the current code.
> 
> The patch is only hardening the code and that should be reflected in the
> commit message.
> 
> So how about:
> 
> xen/arm: domain_build: Harden make_cpus_node()
> 
> make_cpus_node() is using a static buffer to generate the FDT node name.
> 
> While mpdir_aff is a 64-bit integer, we only ever use the bits [23:0] as only
> AFF{0, 1, 2} are supported for now.
> 
> To avoid any potential issue in the future, check that mpdir_aff has only bits
> [23:0] set.
> 
> At the same time, take the opportunity to reduce the size of the buffer.
> Indeed, only 8 characters is useful to generate an 32-bit hexadecimal number.
> So sizeof("cpu@") + 8 = 13 characters is sufficient here.

Ok, thanks for providing the commit message. I'll use it.


> > Fixes: c81a791d34 (xen/arm: Set 'reg' of cpu node for dom0 to match MPIDR's
> > affinity)
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
> > Release-acked-by: Juergen Gross <jgross@suse.com>
> > ---
> > Changes in v3:
> > - make sure only [23:0] bits are used in mpidr_aff
> > - clarify that we only need 32bit for buf writes
> > 
> > Changes in v2:
> > - patch added
> > ---
> >   xen/arch/arm/domain_build.c | 12 +++++++++++-
> >   1 file changed, 11 insertions(+), 1 deletion(-)
> > 
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index 921b054520..d5ee639548 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -789,7 +789,7 @@ static int __init make_cpus_node(const struct domain *d,
> > void *fdt)
> >       const void *compatible = NULL;
> >       u32 len;
> >       /* Placeholder for cpu@ + a 32-bit number + \0 */
> 
> I think you want to update the comment to say "32-bit hexa number".

OK


> > -    char buf[15];
> > +    char buf[13];
> 
> This is a confusing code to read because above you mention this is a 32-bit
> number, but below you are using PRIx64. It takes a bit of time to figure out
> that mpdir_aff will always have bits above 32-bit zeroed.
> 
> I would prefer to use a temporary variable for the register, but I would be
> happy to consider a suitable comment in code.

I'll go with the comment


> >       u32 clock_frequency;
> >       bool clock_valid;
> >       uint64_t mpidr_aff;
> > @@ -847,8 +847,18 @@ static int __init make_cpus_node(const struct domain
> > *d, void *fdt)
> >            * the MPIDR's affinity bits. We will use AFF0 and AFF1 when
> >            * constructing the reg value of the guest at the moment, for it
> >            * is enough for the current max vcpu number.
> > +         *
> > +         * We only deal with AFF{0, 1, 2} stored in bits [23:0] at the
> > +         * moment.
> >            */
> >           mpidr_aff = vcpuid_to_vaffinity(cpu);
> > +        if ( (mpidr_aff & ~GENMASK_ULL(23, 0)) != 0 )
> > +        {
> > +            printk(XENLOG_ERR "Unable to handle MPIDR AFFINITY
> > 0x%"PRIx64"\n",
> > +                   mpidr_aff);
> > +            return -EINVAL;
> > +        }
> > +
> >           dt_dprintk("Create cpu@%"PRIx64" (logical CPUID: %d) node\n",
> >                      mpidr_aff, cpu);
> >   
> 
> Cheers,
> 
> -- 
> Julien Grall
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, back to index

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-08 23:12 [Xen-devel] [PATCH for-4.13 v3] xen/arm: fix buf size in make_cpus_node Stefano Stabellini
2019-10-09 14:48 ` Julien Grall
2019-10-10  0:40   ` Stefano Stabellini

Xen-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/xen-devel/0 xen-devel/git/0.git
	git clone --mirror https://lore.kernel.org/xen-devel/1 xen-devel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 xen-devel xen-devel/ https://lore.kernel.org/xen-devel \
		xen-devel@lists.xenproject.org xen-devel@lists.xen.org xen-devel@archiver.kernel.org
	public-inbox-index xen-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.xenproject.lists.xen-devel


AGPL code for this site: git clone https://public-inbox.org/ public-inbox