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