[PATCH 0/7] Various Spectre-v1 fixes

[PATCH 0/7] Various Spectre-v1 fixes

[Peter Zijlstra]

These fix a number of perf, x86 and sched cases where we have user controlled
array dereferences.  All were found by Dan's recent Smatch update.

[PATCH 1/7] perf: Fix possible Spectre-v1 for aux_pages

[Peter Zijlstra]

[-- Attachment #1: peterz-spectre1-1.patch --]
[-- Type: text/plain, Size: 899 bytes --]

> kernel/events/ring_buffer.c:871 perf_mmap_to_page() warn: potential spectre issue 'rb->aux_pages'

Userspace controls @pgoff through the fault address. Sanitize the
array index before doing the array dereference.

Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Peter Zijlstra <peterz@infradead.org>
---
 kernel/events/ring_buffer.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

--- a/kernel/events/ring_buffer.c
+++ b/kernel/events/ring_buffer.c
@@ -867,8 +867,10 @@ perf_mmap_to_page(struct ring_buffer *rb
 			return NULL;
 
 		/* AUX space */
-		if (pgoff >= rb->aux_pgoff)
-			return virt_to_page(rb->aux_pages[pgoff - rb->aux_pgoff]);
+		if (pgoff >= rb->aux_pgoff) {
+			int aux_pgoff = array_index_nospec(pgoff - rb->aux_pgoff, rb->aux_nr_pages);
+			return virt_to_page(rb->aux_pages[aux_pgoff]);
+		}
 	}
 
 	return __perf_mmap_to_page(rb, pgoff);

[PATCH 2/7] perf,x86: Fix possible Spectre-v1 for hw_perf_event

[Peter Zijlstra]

[-- Attachment #1: peterz-spectre1-2.patch --]
[-- Type: text/plain, Size: 1562 bytes --]

> arch/x86/events/core.c:319 set_ext_hw_attr() warn: potential spectre issue 'hw_cache_event_ids[cache_type]' (local cap)
> arch/x86/events/core.c:319 set_ext_hw_attr() warn: potential spectre issue 'hw_cache_event_ids' (local cap)
> arch/x86/events/core.c:328 set_ext_hw_attr() warn: potential spectre issue 'hw_cache_extra_regs[cache_type]' (local cap)
> arch/x86/events/core.c:328 set_ext_hw_attr() warn: potential spectre issue 'hw_cache_extra_regs' (local cap)

Userspace controls @config which contains 3 (byte) fields used for a 3
dimensional array deref.

Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Peter Zijlstra <peterz@infradead.org>
---
 arch/x86/events/core.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

--- a/arch/x86/events/core.c
+++ b/arch/x86/events/core.c
@@ -304,17 +304,20 @@ set_ext_hw_attr(struct hw_perf_event *hw
 
 	config = attr->config;
 
-	cache_type = (config >>  0) & 0xff;
+	cache_type = (config >> 0) & 0xff;
 	if (cache_type >= PERF_COUNT_HW_CACHE_MAX)
 		return -EINVAL;
+	cache_type = array_index_nospec(cache_type, PERF_COUNT_HW_CACHE_MAX);
 
 	cache_op = (config >>  8) & 0xff;
 	if (cache_op >= PERF_COUNT_HW_CACHE_OP_MAX)
 		return -EINVAL;
+	cache_op = array_index_nospec(cache_op, PERF_COUNT_HW_CACHE_OP_MAX);
 
 	cache_result = (config >> 16) & 0xff;
 	if (cache_result >= PERF_COUNT_HW_CACHE_RESULT_MAX)
 		return -EINVAL;
+	cache_result = array_index_nospec(cache_result, PERF_COUNT_HW_CACHE_RESULT_MAX);
 
 	val = hw_cache_event_ids[cache_type][cache_op][cache_result];
 

[PATCH 3/7] perf,x86: Fix possible Spectre-v1 for x86_pmu::event_map()

[Peter Zijlstra]

[-- Attachment #1: peterz-spectre1-3.patch --]
[-- Type: text/plain, Size: 1098 bytes --]

> arch/x86/events/intel/core.c:337 intel_pmu_event_map() warn: potential spectre issue 'intel_perfmon_event_map'
> arch/x86/events/intel/knc.c:122 knc_pmu_event_map() warn: potential spectre issue 'knc_perfmon_event_map'
> arch/x86/events/intel/p4.c:722 p4_pmu_event_map() warn: potential spectre issue 'p4_general_events'
> arch/x86/events/intel/p6.c:116 p6_pmu_event_map() warn: potential spectre issue 'p6_perfmon_event_map'
> arch/x86/events/amd/core.c:132 amd_pmu_event_map() warn: potential spectre issue 'amd_perfmon_event_map'

Userspace controls @attr, sanitize @attr->config before passing it on
to x86_pmu::event_map().

Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Peter Zijlstra <peterz@infradead.org>
---
 arch/x86/events/core.c |    2 ++
 1 file changed, 2 insertions(+)

--- a/arch/x86/events/core.c
+++ b/arch/x86/events/core.c
@@ -424,6 +424,8 @@ int x86_setup_perfctr(struct perf_event
 	if (attr->config >= x86_pmu.max_events)
 		return -EINVAL;
 
+	attr->config = array_index_nospec(attr->config, x86_pmu.max_events);
+
 	/*
 	 * The generic map:
 	 */

[PATCH 4/7] perf,x86/msr: Fix possible Spectre-v1 for msr

[Peter Zijlstra]

[-- Attachment #1: peterz-spectre1-4.patch --]
[-- Type: text/plain, Size: 1016 bytes --]

> arch/x86/events/msr.c:178 msr_event_init() warn: potential spectre issue 'msr' (local cap)

Userspace controls @attr, sanitize cfg (attr->config) before using it
to index an array.

Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Peter Zijlstra <peterz@infradead.org>
---
 arch/x86/events/msr.c |    8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

--- a/arch/x86/events/msr.c
+++ b/arch/x86/events/msr.c
@@ -158,9 +158,6 @@ static int msr_event_init(struct perf_ev
 	if (event->attr.type != event->pmu->type)
 		return -ENOENT;
 
-	if (cfg >= PERF_MSR_EVENT_MAX)
-		return -EINVAL;
-
 	/* unsupported modes and filters */
 	if (event->attr.exclude_user   ||
 	    event->attr.exclude_kernel ||
@@ -171,6 +168,11 @@ static int msr_event_init(struct perf_ev
 	    event->attr.sample_period) /* no sampling */
 		return -EINVAL;
 
+	if (cfg >= PERF_MSR_EVENT_MAX)
+		return -EINVAL;
+
+	cfg = array_index_nospec(cfg, PERF_MSR_EVENT_MAX);
+
 	if (!msr[cfg].attr)
 		return -EINVAL;
 

[PATCH 5/7] perf,x86/cstate: Fix possible Spectre-v1 for pkg_msr

[Peter Zijlstra]

[-- Attachment #1: peterz-spectre1-5.patch --]
[-- Type: text/plain, Size: 782 bytes --]

> arch/x86/events/intel/cstate.c:307 cstate_pmu_event_init() warn: potential spectre issue 'pkg_msr' (local cap)

Userspace controls @attr, sanitize cfg (attr->config) before using it
to index an array.

Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Peter Zijlstra <peterz@infradead.org>
---
 arch/x86/events/intel/cstate.c |    1 +
 1 file changed, 1 insertion(+)

--- a/arch/x86/events/intel/cstate.c
+++ b/arch/x86/events/intel/cstate.c
@@ -302,6 +302,7 @@ static int cstate_pmu_event_init(struct
 	} else if (event->pmu == &cstate_pkg_pmu) {
 		if (cfg >= PERF_CSTATE_PKG_EVENT_MAX)
 			return -EINVAL;
+		cfg = array_index_nospec(cfg, PERF_CSTATE_PKG_EVENT_MAX);
 		if (!pkg_msr[cfg].attr)
 			return -EINVAL;
 		event->hw.event_base = pkg_msr[cfg].msr;

[PATCH 6/7] sched: Fix possible Spectre-v1 for sched_prio_to_weight[]

[Peter Zijlstra]

[-- Attachment #1: peterz-spectre1-6.patch --]
[-- Type: text/plain, Size: 897 bytes --]

> kernel/sched/core.c:6921 cpu_weight_nice_write_s64() warn: potential spectre issue 'sched_prio_to_weight'

Userspace controls @nice, so sanitize the value before using it to
index an array.

Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Peter Zijlstra <peterz@infradead.org>
---
 kernel/sched/core.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -6928,11 +6928,14 @@ static int cpu_weight_nice_write_s64(str
 				     struct cftype *cft, s64 nice)
 {
 	unsigned long weight;
+	int idx;
 
 	if (nice < MIN_NICE || nice > MAX_NICE)
 		return -ERANGE;
 
-	weight = sched_prio_to_weight[NICE_TO_PRIO(nice) - MAX_RT_PRIO];
+	idx = array_index_nospec(NICE_TO_PRIO(nice) - MAX_RT_PRIO, 40);
+	weight = sched_prio_to_weight[idx];
+
 	return sched_group_set_shares(css_tg(css), scale_load(weight));
 }
 #endif

[PATCH 7/7] sched,autogroup: Fix possible Spectre-v1 for sched_prio_to_weight

[Peter Zijlstra]

[-- Attachment #1: peterz-spectre1-7.patch --]
[-- Type: text/plain, Size: 1026 bytes --]

> kernel/sched/autogroup.c:230 proc_sched_autogroup_set_nice() warn: potential spectre issue 'sched_prio_to_weight'

Userspace controls @nice, sanitize the array index.

Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Peter Zijlstra <peterz@infradead.org>
---
 kernel/sched/autogroup.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

--- a/kernel/sched/autogroup.c
+++ b/kernel/sched/autogroup.c
@@ -209,7 +209,7 @@ int proc_sched_autogroup_set_nice(struct
 	static unsigned long next = INITIAL_JIFFIES;
 	struct autogroup *ag;
 	unsigned long shares;
-	int err;
+	int err, idx;
 
 	if (nice < MIN_NICE || nice > MAX_NICE)
 		return -EINVAL;
@@ -227,7 +227,9 @@ int proc_sched_autogroup_set_nice(struct
 
 	next = HZ / 10 + jiffies;
 	ag = autogroup_task_get(p);
-	shares = scale_load(sched_prio_to_weight[nice + 20]);
+
+	idx = array_index_nospec(nice + 20, 40);
+	shares = scale_load(sched_prio_to_weight[idx]);
 
 	down_write(&ag->lock);
 	err = sched_group_set_shares(ag->tg, shares);

Re: [PATCH 0/7] Various Spectre-v1 fixes

[Michal Hocko]

On Fri 20-04-18 15:14:07, Peter Zijlstra wrote:
> These fix a number of perf, x86 and sched cases where we have user controlled
> array dereferences.  All were found by Dan's recent Smatch update.

Do we want to mark all of these for stable?
-- 
Michal Hocko
SUSE Labs

Re: [PATCH 0/7] Various Spectre-v1 fixes

[Peter Zijlstra]

On Sun, Apr 22, 2018 at 07:03:44PM -0600, Michal Hocko wrote:
> On Fri 20-04-18 15:14:07, Peter Zijlstra wrote:
> > These fix a number of perf, x86 and sched cases where we have user controlled
> > array dereferences.  All were found by Dan's recent Smatch update.
> 
> Do we want to mark all of these for stable?

If we all agree that any (speculative) user-controlled array index --
irrespective of the existence of the second load/store that would
complete the gadget -- needs fixing and thus all these patches are
'good', then yes, that makes sense.

Given yours is the only response so far, I suspect we're good on all
that :-)

Re: [PATCH 0/7] Various Spectre-v1 fixes

[Dan Williams]

On Mon, Apr 23, 2018 at 2:33 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Sun, Apr 22, 2018 at 07:03:44PM -0600, Michal Hocko wrote:
>> On Fri 20-04-18 15:14:07, Peter Zijlstra wrote:
>> > These fix a number of perf, x86 and sched cases where we have user controlled
>> > array dereferences.  All were found by Dan's recent Smatch update.
>>
>> Do we want to mark all of these for stable?
>
> If we all agree that any (speculative) user-controlled array index --
> irrespective of the existence of the second load/store that would
> complete the gadget -- needs fixing and thus all these patches are
> 'good', then yes, that makes sense.
>
> Given yours is the only response so far, I suspect we're good on all
> that :-)

+1 from me for -stable.

Re: [PATCH 0/7] Various Spectre-v1 fixes

[Michal Hocko]

On Mon 23-04-18 11:33:21, Peter Zijlstra wrote:
> On Sun, Apr 22, 2018 at 07:03:44PM -0600, Michal Hocko wrote:
> > On Fri 20-04-18 15:14:07, Peter Zijlstra wrote:
> > > These fix a number of perf, x86 and sched cases where we have user controlled
> > > array dereferences.  All were found by Dan's recent Smatch update.
> > 
> > Do we want to mark all of these for stable?
> 
> If we all agree that any (speculative) user-controlled array index --
> irrespective of the existence of the second load/store that would
> complete the gadget -- needs fixing and thus all these patches are
> 'good', then yes, that makes sense.

Well, I would rather be on the safe side (have I heard security by
fear?). So if those patches are landing in upstream then I would vote to
mark them for stable. They should be trivial to backport and shouldn't
cause regressions that makes them more suitable stable candidates than
many I have seen recently...

-- 
Michal Hocko
SUSE Labs