| Message ID | 20191008231211.31110-1-sstabellini@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [for-4.13,v3] xen/arm: fix buf size in make_cpus_node | expand |
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,
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 >
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);
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(-)