[v1] hugetlb: clean up potential spectre issue warnings

[PATCH] hugetlb: clean up potential spectre issue warnings

Posted by Mike Kravetz 4 years, 4 months ago

Recently introduced code allows numa nodes to be specified on the
kernel command line for hugetlb allocations or CMA reservations.  The
node values are user specified and used as indicies into arrays.  This
generated the following smatch warnings:

mm/hugetlb.c:4170 hugepages_setup() warn: potential spectre issue 'default_hugepages_in_node' [w]
mm/hugetlb.c:4172 hugepages_setup() warn: potential spectre issue 'parsed_hstate->max_huge_pages_node' [w]
mm/hugetlb.c:6898 cmdline_parse_hugetlb_cma() warn: potential spectre issue 'hugetlb_cma_size_in_node' [w] (local cap)

Clean up by using array_index_nospec to sanitize array indicies.

Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
---
 mm/hugetlb.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index 1f0cca036f7f..6b14d0791cb4 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -31,6 +31,7 @@
 #include <linux/llist.h>
 #include <linux/cma.h>
 #include <linux/migrate.h>
+#include <linux/nospec.h>
 
 #include <asm/page.h>
 #include <asm/pgalloc.h>
@@ -4161,7 +4162,7 @@ static int __init hugepages_setup(char *s)
 			}
 			if (tmp >= nr_online_nodes)
 				goto invalid;
-			node = tmp;
+			node = array_index_nospec(tmp, nr_online_nodes);
 			p += count + 1;
 			/* Parse hugepages */
 			if (sscanf(p, "%lu%n", &tmp, &count) != 1)
@@ -6889,9 +6890,9 @@ static int __init cmdline_parse_hugetlb_cma(char *p)
 			break;
 
 		if (s[count] == ':') {
-			nid = tmp;
-			if (nid < 0 || nid >= MAX_NUMNODES)
+			if (tmp < 0 || tmp >= MAX_NUMNODES)
 				break;
+			nid = array_index_nospec(tmp, MAX_NUMNODES);
 
 			s += count + 1;
 			tmp = memparse(s, &s);
-- 
2.34.1

Re: [PATCH] hugetlb: clean up potential spectre issue warnings

Posted by liuyuntao 4 years, 4 months ago

On 17 Feb 2022 15:42:18 -0800, Mike Kravetz wrote:
> Recently introduced code allows numa nodes to be specified on the
> kernel command line for hugetlb allocations or CMA reservations.  The
> node values are user specified and used as indicies into arrays.  This
> generated the following smatch warnings:
> 
> mm/hugetlb.c:4170 hugepages_setup() warn: potential spectre issue 'default_hugepages_in_node' [w]
> mm/hugetlb.c:4172 hugepages_setup() warn: potential spectre issue 'parsed_hstate->max_huge_pages_node' [w]
> mm/hugetlb.c:6898 cmdline_parse_hugetlb_cma() warn: potential spectre issue 'hugetlb_cma_size_in_node' [w] (local cap)
> 
> Clean up by using array_index_nospec to sanitize array indicies.
> 
> Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
> ---
>  mm/hugetlb.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index 1f0cca036f7f..6b14d0791cb4 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -31,6 +31,7 @@
>  #include <linux/llist.h>
>  #include <linux/cma.h>
>  #include <linux/migrate.h>
> +#include <linux/nospec.h>
>  
>  #include <asm/page.h>
>  #include <asm/pgalloc.h>
> @@ -4161,7 +4162,7 @@ static int __init hugepages_setup(char *s)
>  			}
>  			if (tmp >= nr_online_nodes)
>  				goto invalid;
> -			node = tmp;
> +			node = array_index_nospec(tmp, nr_online_nodes);
>  			p += count + 1;
>  			/* Parse hugepages */
>  			if (sscanf(p, "%lu%n", &tmp, &count) != 1)
> @@ -6889,9 +6890,9 @@ static int __init cmdline_parse_hugetlb_cma(char *p)
>  			break;
>  
>  		if (s[count] == ':') {
> -			nid = tmp;
> -			if (nid < 0 || nid >= MAX_NUMNODES)
> +			if (tmp < 0 || tmp >= MAX_NUMNODES)

Here tmp is unsigned, no need to check if less than 0.
Do we really have any automated checking? lol

>  				break;
> +			nid = array_index_nospec(tmp, MAX_NUMNODES);
>  
>  			s += count + 1;
>  			tmp = memparse(s, &s);
> -- 
> 2.34.1

Re: [PATCH] hugetlb: clean up potential spectre issue warnings

Posted by Dan Carpenter 4 years, 4 months ago

On Fri, Feb 18, 2022 at 11:40:25AM +0800, liuyuntao wrote:
> On 17 Feb 2022 15:42:18 -0800, Mike Kravetz wrote:
> > Recently introduced code allows numa nodes to be specified on the
> > kernel command line for hugetlb allocations or CMA reservations.  The
> > node values are user specified and used as indicies into arrays.  This
> > generated the following smatch warnings:
> > 
> > mm/hugetlb.c:4170 hugepages_setup() warn: potential spectre issue 'default_hugepages_in_node' [w]
> > mm/hugetlb.c:4172 hugepages_setup() warn: potential spectre issue 'parsed_hstate->max_huge_pages_node' [w]
> > mm/hugetlb.c:6898 cmdline_parse_hugetlb_cma() warn: potential spectre issue 'hugetlb_cma_size_in_node' [w] (local cap)
> > 
> > Clean up by using array_index_nospec to sanitize array indicies.
> > 
> > Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
> > ---
> >  mm/hugetlb.c | 7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> > 
> > diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> > index 1f0cca036f7f..6b14d0791cb4 100644
> > --- a/mm/hugetlb.c
> > +++ b/mm/hugetlb.c
> > @@ -31,6 +31,7 @@
> >  #include <linux/llist.h>
> >  #include <linux/cma.h>
> >  #include <linux/migrate.h>
> > +#include <linux/nospec.h>
> >  
> >  #include <asm/page.h>
> >  #include <asm/pgalloc.h>
> > @@ -4161,7 +4162,7 @@ static int __init hugepages_setup(char *s)
> >  			}
> >  			if (tmp >= nr_online_nodes)
> >  				goto invalid;
> > -			node = tmp;
> > +			node = array_index_nospec(tmp, nr_online_nodes);
> >  			p += count + 1;
> >  			/* Parse hugepages */
> >  			if (sscanf(p, "%lu%n", &tmp, &count) != 1)
> > @@ -6889,9 +6890,9 @@ static int __init cmdline_parse_hugetlb_cma(char *p)
> >  			break;
> >  
> >  		if (s[count] == ':') {
> > -			nid = tmp;
> > -			if (nid < 0 || nid >= MAX_NUMNODES)
> > +			if (tmp < 0 || tmp >= MAX_NUMNODES)
> 
> Here tmp is unsigned, no need to check if less than 0.
> Do we really have any automated checking? lol
> 

Smatch ignores checks for negative when it's part of a clamp test.  In
this situation the check for negative is obviously harmless so a warning
is a false positive.

If you wrote it the other way:

		if (tmp >= MAX_NUMNODES || tmp < 0)

then Smatch would print a warning because I try not to get involved with
style debates but I really don't like that style...  :P

regards,
dan carpenter

Re: [PATCH] hugetlb: clean up potential spectre issue warnings

Posted by Mike Kravetz 4 years, 4 months ago

On 2/17/22 19:40, liuyuntao wrote:
> On 17 Feb 2022 15:42:18 -0800, Mike Kravetz wrote:
>> Recently introduced code allows numa nodes to be specified on the
>> kernel command line for hugetlb allocations or CMA reservations.  The
>> node values are user specified and used as indicies into arrays.  This
>> generated the following smatch warnings:
>>
>> mm/hugetlb.c:4170 hugepages_setup() warn: potential spectre issue 'default_hugepages_in_node' [w]
>> mm/hugetlb.c:4172 hugepages_setup() warn: potential spectre issue 'parsed_hstate->max_huge_pages_node' [w]
>> mm/hugetlb.c:6898 cmdline_parse_hugetlb_cma() warn: potential spectre issue 'hugetlb_cma_size_in_node' [w] (local cap)
>>
>> Clean up by using array_index_nospec to sanitize array indicies.
>>
>> Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
>> ---
>>  mm/hugetlb.c | 7 ++++---
>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
>> index 1f0cca036f7f..6b14d0791cb4 100644
>> --- a/mm/hugetlb.c
>> +++ b/mm/hugetlb.c
>> @@ -31,6 +31,7 @@
>>  #include <linux/llist.h>
>>  #include <linux/cma.h>
>>  #include <linux/migrate.h>
>> +#include <linux/nospec.h>
>>  
>>  #include <asm/page.h>
>>  #include <asm/pgalloc.h>
>> @@ -4161,7 +4162,7 @@ static int __init hugepages_setup(char *s)
>>  			}
>>  			if (tmp >= nr_online_nodes)
>>  				goto invalid;
>> -			node = tmp;
>> +			node = array_index_nospec(tmp, nr_online_nodes);
>>  			p += count + 1;
>>  			/* Parse hugepages */
>>  			if (sscanf(p, "%lu%n", &tmp, &count) != 1)
>> @@ -6889,9 +6890,9 @@ static int __init cmdline_parse_hugetlb_cma(char *p)
>>  			break;
>>  
>>  		if (s[count] == ':') {
>> -			nid = tmp;
>> -			if (nid < 0 || nid >= MAX_NUMNODES)
>> +			if (tmp < 0 || tmp >= MAX_NUMNODES)
> 
> Here tmp is unsigned, no need to check if less than 0.

Thanks!

I shuffled the code a bit and missed this point.  Actually, this routine
cmdline_parse_hugetlb_cma has the same issue you addressed in [1].  I did not
see that until now.  It is fixed with this change.

I will send a v2 with:
- Remove check for unsigned tmp < 0
- Add a note in commit log that this also addresses an overflow truncation
  issue in the assignement of an unsigned long to int.

[1] https://lore.kernel.org/linux-mm/20220209134018.8242-1-liuyuntao10@huawei.com/
-- 
Mike Kravetz