Re: [Xen-devel] [PATCH for-4.13 v3] xen/arm: fix buf size in make_cpus_node
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
> > Release-acked-by: Juergen Gross
> > ---
> > 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
Re: [Xen-devel] [PATCH for-4.13 v3] xen/arm: fix buf size in make_cpus_node
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
Release-acked-by: Juergen Gross
---
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
[Xen-devel] [PATCH for-4.13 v3] xen/arm: fix buf size in make_cpus_node
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
Release-acked-by: Juergen Gross
---
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
