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

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

[Mike Kravetz]

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.

The routine cmdline_parse_hugetlb_cma has the same overflow/truncation issue
addressed in [1].  That is also fixed with this change.

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

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..55abf4e31603 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 >= MAX_NUMNODES)
 				break;
+			nid = array_index_nospec(tmp, MAX_NUMNODES);
 
 			s += count + 1;
 			tmp = memparse(s, &s);
-- 
2.34.1

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

[Michal Hocko]

On Fri 18-02-22 13:29:46, Mike Kravetz wrote:
[...]
> @@ -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 >= MAX_NUMNODES)
>  				break;
> +			nid = array_index_nospec(tmp, MAX_NUMNODES);
>  
>  			s += count + 1;
>  			tmp = memparse(s, &s);

This is an early boot code, how is this supposed to be used as a side
channel?
-- 
Michal Hocko
SUSE Labs

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

[Mike Kravetz]

On 2/21/22 00:42, Michal Hocko wrote:
> On Fri 18-02-22 13:29:46, Mike Kravetz wrote:
> [...]
>> @@ -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 >= MAX_NUMNODES)
>>  				break;
>> +			nid = array_index_nospec(tmp, MAX_NUMNODES);
>>  
>>  			s += count + 1;
>>  			tmp = memparse(s, &s);
> 
> This is an early boot code, how is this supposed to be used as a side
> channel?

I do not have an evil hacker mind, but I can not think of a way this one time
use of a user specified index could be an issue.  It does add noise to the
BUILD REGRESSION emails sent to Andrew.

-- 
Mike Kravetz

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

[Michal Hocko]

On Mon 21-02-22 12:24:25, Mike Kravetz wrote:
> On 2/21/22 00:42, Michal Hocko wrote:
> > On Fri 18-02-22 13:29:46, Mike Kravetz wrote:
> > [...]
> >> @@ -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 >= MAX_NUMNODES)
> >>  				break;
> >> +			nid = array_index_nospec(tmp, MAX_NUMNODES);
> >>  
> >>  			s += count + 1;
> >>  			tmp = memparse(s, &s);
> > 
> > This is an early boot code, how is this supposed to be used as a side
> > channel?
> 
> I do not have an evil hacker mind, but I can not think of a way this one time
> use of a user specified index could be an issue.  It does add noise to the
> BUILD REGRESSION emails sent to Andrew.

Maybe Smack can be taught to ignore __init and other early boot
functions.

I do not have any strong objections to using array_index_nospec because
it won't do any harm. Except that it makes a security measure a normal
comodity so any future changes to array_index_nospec and its users will
have to consult additional callers. Whether that is something we should
deeply care about, I don't know.

At minimum make sure to be explicit that this can hardly be a Spectre
gadget as it is a _one_ time early boot call. If there is a scenario
where this could be really abused then it should be mentioned
explicitly.
-- 
Michal Hocko
SUSE Labs

RE: [PATCH v2] hugetlb: clean up potential spectre issue warnings

[Schaufler, Casey]

> -----Original Message-----
> From: Michal Hocko <mhocko@suse.com>
> Sent: Monday, February 21, 2022 11:48 PM
> To: Mike Kravetz <mike.kravetz@oracle.com>
> Cc: linux-mm@kvack.org; linux-kernel@vger.kernel.org; Baolin Wang
> <baolin.wang@linux.alibaba.com>; Zhenguo Yao
> <yaozhenguo1@gmail.com>; Liu Yuntao <liuyuntao10@huawei.com>; Dan
> Carpenter <dan.carpenter@oracle.com>; Andrew Morton <akpm@linux-
> foundation.org>
> Subject: Re: [PATCH v2] hugetlb: clean up potential spectre issue warnings
> 
> On Mon 21-02-22 12:24:25, Mike Kravetz wrote:
> > On 2/21/22 00:42, Michal Hocko wrote:
> > > On Fri 18-02-22 13:29:46, Mike Kravetz wrote:
> > > [...]
> > >> @@ -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 >= MAX_NUMNODES)
> > >>  				break;
> > >> +			nid = array_index_nospec(tmp, MAX_NUMNODES);
> > >>
> > >>  			s += count + 1;
> > >>  			tmp = memparse(s, &s);
> > >
> > > This is an early boot code, how is this supposed to be used as a side
> > > channel?
> >
> > I do not have an evil hacker mind, but I can not think of a way this one time
> > use of a user specified index could be an issue.  It does add noise to the
> > BUILD REGRESSION emails sent to Andrew.
> 
> Maybe Smack can be taught to ignore __init and other early boot
> functions.

Why is Smack getting called out? The relationship is not obvious.

> 
> I do not have any strong objections to using array_index_nospec because
> it won't do any harm. Except that it makes a security measure a normal
> comodity so any future changes to array_index_nospec and its users will
> have to consult additional callers. Whether that is something we should
> deeply care about, I don't know.
> 
> At minimum make sure to be explicit that this can hardly be a Spectre
> gadget as it is a _one_ time early boot call. If there is a scenario
> where this could be really abused then it should be mentioned
> explicitly.
> --
> Michal Hocko
> SUSE Labs

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

[Dan Carpenter]

On Tue, Feb 22, 2022 at 04:15:45PM +0000, Schaufler, Casey wrote:
> > -----Original Message-----
> > From: Michal Hocko <mhocko@suse.com>
> > Sent: Monday, February 21, 2022 11:48 PM
> > To: Mike Kravetz <mike.kravetz@oracle.com>
> > Cc: linux-mm@kvack.org; linux-kernel@vger.kernel.org; Baolin Wang
> > <baolin.wang@linux.alibaba.com>; Zhenguo Yao
> > <yaozhenguo1@gmail.com>; Liu Yuntao <liuyuntao10@huawei.com>; Dan
> > Carpenter <dan.carpenter@oracle.com>; Andrew Morton <akpm@linux-
> > foundation.org>
> > Subject: Re: [PATCH v2] hugetlb: clean up potential spectre issue warnings
> > 
> > On Mon 21-02-22 12:24:25, Mike Kravetz wrote:
> > > On 2/21/22 00:42, Michal Hocko wrote:
> > > > On Fri 18-02-22 13:29:46, Mike Kravetz wrote:
> > > > [...]
> > > >> @@ -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 >= MAX_NUMNODES)
> > > >>  				break;
> > > >> +			nid = array_index_nospec(tmp, MAX_NUMNODES);
> > > >>
> > > >>  			s += count + 1;
> > > >>  			tmp = memparse(s, &s);
> > > >
> > > > This is an early boot code, how is this supposed to be used as a side
> > > > channel?
> > >
> > > I do not have an evil hacker mind, but I can not think of a way this one time
> > > use of a user specified index could be an issue.  It does add noise to the
> > > BUILD REGRESSION emails sent to Andrew.
> > 
> > Maybe Smack can be taught to ignore __init and other early boot
> > functions.
> 
> Why is Smack getting called out? The relationship is not obvious.
> 

He meant Smatch.  It's a really common mistake that I did not anticipate
in 2002.

I can probably silence the spectre warnings for __init functions.  TBH,
I don't really understand spectre at all so I mostly ignore those
warnings.  :/

regards,
dan carpenter

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

[Michal Hocko]

On Tue 22-02-22 19:36:10, Dan Carpenter wrote:
> On Tue, Feb 22, 2022 at 04:15:45PM +0000, Schaufler, Casey wrote:
> > > -----Original Message-----
> > > From: Michal Hocko <mhocko@suse.com>
> > > Sent: Monday, February 21, 2022 11:48 PM
> > > To: Mike Kravetz <mike.kravetz@oracle.com>
> > > Cc: linux-mm@kvack.org; linux-kernel@vger.kernel.org; Baolin Wang
> > > <baolin.wang@linux.alibaba.com>; Zhenguo Yao
> > > <yaozhenguo1@gmail.com>; Liu Yuntao <liuyuntao10@huawei.com>; Dan
> > > Carpenter <dan.carpenter@oracle.com>; Andrew Morton <akpm@linux-
> > > foundation.org>
> > > Subject: Re: [PATCH v2] hugetlb: clean up potential spectre issue warnings
> > > 
> > > On Mon 21-02-22 12:24:25, Mike Kravetz wrote:
> > > > On 2/21/22 00:42, Michal Hocko wrote:
> > > > > On Fri 18-02-22 13:29:46, Mike Kravetz wrote:
> > > > > [...]
> > > > >> @@ -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 >= MAX_NUMNODES)
> > > > >>  				break;
> > > > >> +			nid = array_index_nospec(tmp, MAX_NUMNODES);
> > > > >>
> > > > >>  			s += count + 1;
> > > > >>  			tmp = memparse(s, &s);
> > > > >
> > > > > This is an early boot code, how is this supposed to be used as a side
> > > > > channel?
> > > >
> > > > I do not have an evil hacker mind, but I can not think of a way this one time
> > > > use of a user specified index could be an issue.  It does add noise to the
> > > > BUILD REGRESSION emails sent to Andrew.
> > > 
> > > Maybe Smack can be taught to ignore __init and other early boot
> > > functions.
> > 
> > Why is Smack getting called out? The relationship is not obvious.
> > 
> 
> He meant Smatch.  It's a really common mistake that I did not anticipate
> in 2002.

Right. My bad.

> I can probably silence the spectre warnings for __init functions.  TBH,
> I don't really understand spectre at all so I mostly ignore those
> warnings.  :/

AFAIU the spectre gadget would need to be called repeatedly to be usable
for a side channel attack. I might be really missing some scenario but
to me it seems that __init functions cannot really be used for that.
-- 
Michal Hocko
SUSE Labs

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

[Mike Kravetz]

On 2/21/22 23:47, Michal Hocko wrote:
> On Mon 21-02-22 12:24:25, Mike Kravetz wrote:
>> On 2/21/22 00:42, Michal Hocko wrote:
>>> On Fri 18-02-22 13:29:46, Mike Kravetz wrote:
>>> [...]
>>>> @@ -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 >= MAX_NUMNODES)
>>>>  				break;
>>>> +			nid = array_index_nospec(tmp, MAX_NUMNODES);
>>>>  
>>>>  			s += count + 1;
>>>>  			tmp = memparse(s, &s);
>>>
>>> This is an early boot code, how is this supposed to be used as a side
>>> channel?
>>
>> I do not have an evil hacker mind, but I can not think of a way this one time
>> use of a user specified index could be an issue.  It does add noise to the
>> BUILD REGRESSION emails sent to Andrew.
> 
> Maybe Smack can be taught to ignore __init and other early boot
> functions.
> 
> I do not have any strong objections to using array_index_nospec because
> it won't do any harm. Except that it makes a security measure a normal
> comodity so any future changes to array_index_nospec and its users will
> have to consult additional callers. Whether that is something we should
> deeply care about, I don't know.
> 
> At minimum make sure to be explicit that this can hardly be a Spectre
> gadget as it is a _one_ time early boot call. If there is a scenario
> where this could be really abused then it should be mentioned
> explicitly.

How about adding this note to the commit message?

Note: these routines take a user specified value used as an index ONCE
during the boot process.  As a result, they can not be used as a general
method of exploitation.  Code changes are being made to eliminate warnings.

Andrew, would you like me to send a v3?
-- 
Mike Kravetz

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

[Michal Hocko]

On Tue 22-02-22 13:53:56, Mike Kravetz wrote:
> On 2/21/22 23:47, Michal Hocko wrote:
> > On Mon 21-02-22 12:24:25, Mike Kravetz wrote:
> >> On 2/21/22 00:42, Michal Hocko wrote:
> >>> On Fri 18-02-22 13:29:46, Mike Kravetz wrote:
> >>> [...]
> >>>> @@ -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 >= MAX_NUMNODES)
> >>>>  				break;
> >>>> +			nid = array_index_nospec(tmp, MAX_NUMNODES);
> >>>>  
> >>>>  			s += count + 1;
> >>>>  			tmp = memparse(s, &s);
> >>>
> >>> This is an early boot code, how is this supposed to be used as a side
> >>> channel?
> >>
> >> I do not have an evil hacker mind, but I can not think of a way this one time
> >> use of a user specified index could be an issue.  It does add noise to the
> >> BUILD REGRESSION emails sent to Andrew.
> > 
> > Maybe Smack can be taught to ignore __init and other early boot
> > functions.
> > 
> > I do not have any strong objections to using array_index_nospec because
> > it won't do any harm. Except that it makes a security measure a normal
> > comodity so any future changes to array_index_nospec and its users will
> > have to consult additional callers. Whether that is something we should
> > deeply care about, I don't know.
> > 
> > At minimum make sure to be explicit that this can hardly be a Spectre
> > gadget as it is a _one_ time early boot call. If there is a scenario
> > where this could be really abused then it should be mentioned
> > explicitly.
> 
> How about adding this note to the commit message?
> 
> Note: these routines take a user specified value used as an index ONCE
> during the boot process.  As a result, they can not be used as a general
> method of exploitation.  Code changes are being made to eliminate warnings.

This would help but the question whether the change is worth remains.
Does this change have any other advantage than silencing the warning?

-- 
Michal Hocko
SUSE Labs

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

[Mike Kravetz]

On 2/23/22 00:33, Michal Hocko wrote:
> On Tue 22-02-22 13:53:56, Mike Kravetz wrote:
>> On 2/21/22 23:47, Michal Hocko wrote:
>> How about adding this note to the commit message?
>>
>> Note: these routines take a user specified value used as an index ONCE
>> during the boot process.  As a result, they can not be used as a general
>> method of exploitation.  Code changes are being made to eliminate warnings.
> 
> This would help but the question whether the change is worth remains.
> Does this change have any other advantage than silencing the warning?
> 

Silencing the warnings was the primary motivation for the change.  If Dan
has a plan to change smatch so that they are silenced for __init functions,
then it would be better to not make the changes to use array_index_nospec.

While making the changes, I shuffled the code a little and did not immediately
notice that it also 'fixes' an overflow/truncation issue when assigning an
unsigned long to int as addressed in [1].  We should probably make this change
whether or not we use array_index_nospec to silence warnings.

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

Thanks for your comments and suggestions!
-- 
Mike Kravetz

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

[Michal Hocko]

On Wed 23-02-22 10:36:55, Mike Kravetz wrote:
> On 2/23/22 00:33, Michal Hocko wrote:
> > On Tue 22-02-22 13:53:56, Mike Kravetz wrote:
> >> On 2/21/22 23:47, Michal Hocko wrote:
> >> How about adding this note to the commit message?
> >>
> >> Note: these routines take a user specified value used as an index ONCE
> >> during the boot process.  As a result, they can not be used as a general
> >> method of exploitation.  Code changes are being made to eliminate warnings.
> > 
> > This would help but the question whether the change is worth remains.
> > Does this change have any other advantage than silencing the warning?
> > 
> 
> Silencing the warnings was the primary motivation for the change.  If Dan
> has a plan to change smatch so that they are silenced for __init functions,
> then it would be better to not make the changes to use array_index_nospec.
> 
> While making the changes, I shuffled the code a little and did not immediately
> notice that it also 'fixes' an overflow/truncation issue when assigning an
> unsigned long to int as addressed in [1].  We should probably make this change
> whether or not we use array_index_nospec to silence warnings.
> 
> [1] https://lore.kernel.org/linux-mm/20220209134018.8242-1-liuyuntao10@huawei.com/

Yeah, this makes sense to me.
-- 
Michal Hocko
SUSE Labs

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

[Andrew Morton]

On Tue, 22 Feb 2022 13:53:56 -0800 Mike Kravetz <mike.kravetz@oracle.com> wrote:

> > gadget as it is a _one_ time early boot call. If there is a scenario
> > where this could be really abused then it should be mentioned
> > explicitly.
> 
> How about adding this note to the commit message?

I have already done that.  I added

"As Michal pointed out, this is unlikely to be exploitable because it is
__init code.  But the patch suppresses the warnings."