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
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
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
© 2016 - 2024 Red Hat, Inc.