| Message ID | 20220218212946.35441-1-mike.kravetz@oracle.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] hugetlb: clean up potential spectre issue warnings | expand |
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?
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.
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.
> -----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
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
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?
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?
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.
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!
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."
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.
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);
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(-)