1. Patchew
  2. linux
  3. View series

[PATCH v2] topology: Keep the cpumask unchanged when printing cpumap

Li Huafei posted 1 patch 1 week, 1 day ago 
drivers/base/topology.c | 24 ++++++++++++++++++++----
1 file changed, 20 insertions(+), 4 deletions(-)
[PATCH v2] topology: Keep the cpumask unchanged when printing cpumap
Posted by Li Huafei 1 week, 1 day ago 
During fuzz testing, the following warning was discovered:

 different return values (15 and 11) from vsnprintf("%*pbl
 ", ...)

 test:keyward is WARNING in kvasprintf
 WARNING: CPU: 55 PID: 1168477 at lib/kasprintf.c:30 kvasprintf+0x121/0x130
 Call Trace:
  kvasprintf+0x121/0x130
  kasprintf+0xa6/0xe0
  bitmap_print_to_buf+0x89/0x100
  core_siblings_list_read+0x7e/0xb0
  kernfs_file_read_iter+0x15b/0x270
  new_sync_read+0x153/0x260
  vfs_read+0x215/0x290
  ksys_read+0xb9/0x160
  do_syscall_64+0x56/0x100
  entry_SYSCALL_64_after_hwframe+0x78/0xe2

The call trace shows that kvasprintf() reported this warning during the
printing of core_siblings_list. kvasprintf() has several steps:

 (1) First, calculate the length of the resulting formatted string.

 (2) Allocate a buffer based on the returned length.

 (3) Then, perform the actual string formatting.

 (4) Check whether the lengths of the formatted strings returned in
     steps (1) and (2) are consistent.

If the core_cpumask is modified between steps (1) and (3), the lengths
obtained in these two steps may not match. Indeed our test includes cpu
hotplugging, which should modify core_cpumask while printing.

To fix this issue, cache the cpumask into a temporary variable before
calling cpumap_print_{list, cpumask}_to_buf(), to keep it unchanged
during the printing process.

Fixes: bb9ec13d156e ("topology: use bin_attribute to break the size limitation of cpumap ABI")
Cc: stable@vger.kernel.org
Signed-off-by: Li Huafei <lihuafei1@huawei.com>
---
Changes in v2:
 - Return an error when calling alloc_cpumask_var() fails instead of
   returning a size of 0. 
 - Add Cc (to stable) tag.
---
 drivers/base/topology.c | 24 ++++++++++++++++++++----
 1 file changed, 20 insertions(+), 4 deletions(-)

diff --git a/drivers/base/topology.c b/drivers/base/topology.c
index 89f98be5c5b9..d293cbd253e4 100644
--- a/drivers/base/topology.c
+++ b/drivers/base/topology.c
@@ -27,9 +27,17 @@ static ssize_t name##_read(struct file *file, struct kobject *kobj,		\
 			   loff_t off, size_t count)				\
 {										\
 	struct device *dev = kobj_to_dev(kobj);                                 \
+	cpumask_var_t mask;							\
+	ssize_t n;								\
 										\
-	return cpumap_print_bitmask_to_buf(buf, topology_##mask(dev->id),	\
-					   off, count);                         \
+	if (!alloc_cpumask_var(&mask, GFP_KERNEL))				\
+		return -ENOMEM;							\
+										\
+	cpumask_copy(mask, topology_##mask(dev->id));				\
+	n = cpumap_print_bitmask_to_buf(buf, mask, off, count);			\
+	free_cpumask_var(mask);							\
+										\
+	return n;								\
 }										\
 										\
 static ssize_t name##_list_read(struct file *file, struct kobject *kobj,	\
@@ -37,9 +45,17 @@ static ssize_t name##_list_read(struct file *file, struct kobject *kobj,	\
 				loff_t off, size_t count)			\
 {										\
 	struct device *dev = kobj_to_dev(kobj);					\
+	cpumask_var_t mask;							\
+	ssize_t n;								\
+										\
+	if (!alloc_cpumask_var(&mask, GFP_KERNEL))				\
+		return -ENOMEM;							\
+										\
+	cpumask_copy(mask, topology_##mask(dev->id));				\
+	n = cpumap_print_list_to_buf(buf, mask, off, count);			\
+	free_cpumask_var(mask);							\
 										\
-	return cpumap_print_list_to_buf(buf, topology_##mask(dev->id),		\
-					off, count);				\
+	return n;								\
 }
 
 define_id_show_func(physical_package_id, "%d");
-- 
2.25.1
Re: [PATCH v2] topology: Keep the cpumask unchanged when printing cpumap
Posted by Jonathan Cameron 1 week ago 
On Thu, 14 Nov 2024 19:01:41 +0800
Li Huafei <lihuafei1@huawei.com> wrote:

> During fuzz testing, the following warning was discovered:
> 
>  different return values (15 and 11) from vsnprintf("%*pbl
>  ", ...)
> 
>  test:keyward is WARNING in kvasprintf
>  WARNING: CPU: 55 PID: 1168477 at lib/kasprintf.c:30 kvasprintf+0x121/0x130
>  Call Trace:
>   kvasprintf+0x121/0x130
>   kasprintf+0xa6/0xe0
>   bitmap_print_to_buf+0x89/0x100
>   core_siblings_list_read+0x7e/0xb0
>   kernfs_file_read_iter+0x15b/0x270
>   new_sync_read+0x153/0x260
>   vfs_read+0x215/0x290
>   ksys_read+0xb9/0x160
>   do_syscall_64+0x56/0x100
>   entry_SYSCALL_64_after_hwframe+0x78/0xe2
> 
> The call trace shows that kvasprintf() reported this warning during the
> printing of core_siblings_list. kvasprintf() has several steps:
> 
>  (1) First, calculate the length of the resulting formatted string.
> 
>  (2) Allocate a buffer based on the returned length.
> 
>  (3) Then, perform the actual string formatting.
> 
>  (4) Check whether the lengths of the formatted strings returned in
>      steps (1) and (2) are consistent.
> 
> If the core_cpumask is modified between steps (1) and (3), the lengths
> obtained in these two steps may not match. Indeed our test includes cpu
> hotplugging, which should modify core_cpumask while printing.
> 
> To fix this issue, cache the cpumask into a temporary variable before
> calling cpumap_print_{list, cpumask}_to_buf(), to keep it unchanged
> during the printing process.
> 
> Fixes: bb9ec13d156e ("topology: use bin_attribute to break the size limitation of cpumap ABI")
> Cc: stable@vger.kernel.org
> Signed-off-by: Li Huafei <lihuafei1@huawei.com>
Makes sense. Trivial comment inline.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
> Changes in v2:
>  - Return an error when calling alloc_cpumask_var() fails instead of
>    returning a size of 0. 
>  - Add Cc (to stable) tag.
> ---
>  drivers/base/topology.c | 24 ++++++++++++++++++++----
>  1 file changed, 20 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/base/topology.c b/drivers/base/topology.c
> index 89f98be5c5b9..d293cbd253e4 100644
> --- a/drivers/base/topology.c
> +++ b/drivers/base/topology.c
> @@ -27,9 +27,17 @@ static ssize_t name##_read(struct file *file, struct kobject *kobj,		\
>  			   loff_t off, size_t count)				\
>  {										\
>  	struct device *dev = kobj_to_dev(kobj);                                 \
> +	cpumask_var_t mask;							\
> +	ssize_t n;								\
>  										\
> -	return cpumap_print_bitmask_to_buf(buf, topology_##mask(dev->id),	\
> -					   off, count);                         \
> +	if (!alloc_cpumask_var(&mask, GFP_KERNEL))				\
> +		return -ENOMEM;							\
Good catch.
Could use __free(free_cpumask_var) but that is a bit messy given it's not a conventional
allocation that returns a pointer.  So probably not worth doing just to save a single
manual free call.


> +										\
> +	cpumask_copy(mask, topology_##mask(dev->id));				\
> +	n = cpumap_print_bitmask_to_buf(buf, mask, off, count);			\
> +	free_cpumask_var(mask);							\
> +										\
> +	return n;								\
>  }										\
>  										\
>  static ssize_t name##_list_read(struct file *file, struct kobject *kobj,	\
> @@ -37,9 +45,17 @@ static ssize_t name##_list_read(struct file *file, struct kobject *kobj,	\
>  				loff_t off, size_t count)			\
>  {										\
>  	struct device *dev = kobj_to_dev(kobj);					\
> +	cpumask_var_t mask;							\
> +	ssize_t n;								\
> +										\
> +	if (!alloc_cpumask_var(&mask, GFP_KERNEL))				\
> +		return -ENOMEM;							\
> +										\
> +	cpumask_copy(mask, topology_##mask(dev->id));				\
> +	n = cpumap_print_list_to_buf(buf, mask, off, count);			\
> +	free_cpumask_var(mask);							\
>  										\
> -	return cpumap_print_list_to_buf(buf, topology_##mask(dev->id),		\
> -					off, count);				\
> +	return n;								\
>  }
>  
>  define_id_show_func(physical_package_id, "%d");

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.