diff mbox series

[v2] xen: hypercall: fix out-of-bounds memcpy

Message ID 20180205150340.328921-1-arnd@arndb.de
State New
Headers show
Series [v2] xen: hypercall: fix out-of-bounds memcpy | expand

Commit Message

Arnd Bergmann Feb. 5, 2018, 3:03 p.m. UTC 
The legacy hypercall handlers were originally added with
a comment explaining that "copying the argument structures in
HYPERVISOR_event_channel_op() and HYPERVISOR_physdev_op() into the local
variable is sufficiently safe" and only made sure to not write
past the end of the argument structure, the checks in linux/string.h
disagree with that, when link-time optimizations are used:

In function 'memcpy',
    inlined from 'pirq_query_unmask' at drivers/xen/fallback.c:53:2,
    inlined from '__startup_pirq' at drivers/xen/events/events_base.c:529:2,
    inlined from 'restore_pirqs' at drivers/xen/events/events_base.c:1439:3,
    inlined from 'xen_irq_resume' at drivers/xen/events/events_base.c:1581:2:
include/linux/string.h:350:3: error: call to '__read_overflow2' declared with attribute error: detected read beyond size of object passed as 2nd parameter
   __read_overflow2();
   ^
make[3]: *** [ccLujFNx.ltrans15.ltrans.o] Error 1
make[3]: Target 'all' not remade because of errors.
lto-wrapper: fatal error: make returned 2 exit status
compilation terminated.
ld: error: lto-wrapper failed

This changes the functions so that each argument is accessed with
exactly the correct length based on the command code.

Fixes: cf47a83fb06e ("xen/hypercall: fix hypercall fallback code for very old hypervisors")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>

---
[v2] use a table lookup instead of a switch/case statement, after
multiple suggestions.
---
 drivers/xen/fallback.c | 94 +++++++++++++++++++++-----------------------------
 1 file changed, 39 insertions(+), 55 deletions(-)

-- 
2.9.0

Comments

Andrew Cooper Feb. 5, 2018, 3:14 p.m. UTC | #1 
On 05/02/18 15:03, Arnd Bergmann wrote:

Snipping deleted code to make things clearer:

> +	if (cmd > ARRAY_SIZE(physdevop_len))

> +		return -ENOSYS;

>

> +	len = physdevop_len[cmd];

> +	memcpy(&op.u, arg, len);


You'll want an array_nospec() or whatever its called these days.  This
code is SP1-leaky.

Userspace controls cmd and can retrieve len by timing how many adjacent
cache lines were pulled in my memcpy().

~Andrew
Jan Beulich Feb. 5, 2018, 3:14 p.m. UTC | #2 
>>> On 05.02.18 at 16:03, <arnd@arndb.de> wrote:

>  int xen_event_channel_op_compat(int cmd, void *arg)

>  {

> -	struct evtchn_op op;

> +	struct evtchn_op op = { .cmd = cmd, };

> +	size_t len;

>  	int rc;

>  

> -	op.cmd = cmd;

> -	memcpy(&op.u, arg, sizeof(op.u));

> -	rc = _hypercall1(int, event_channel_op_compat, &op);

> -

> -	switch (cmd) {

> -	case EVTCHNOP_close:

> -	case EVTCHNOP_send:

> -	case EVTCHNOP_bind_vcpu:

> -	case EVTCHNOP_unmask:

> -		/* no output */

> -		break;

> +	if (cmd > ARRAY_SIZE(evtchnop_len))

> +		return -ENOSYS;


>= perhaps?


Jan
David Laight Feb. 5, 2018, 3:28 p.m. UTC | #3 
From: Andrew Cooper

> Sent: 05 February 2018 15:14

> 

> On 05/02/18 15:03, Arnd Bergmann wrote:

> 

> Snipping deleted code to make things clearer:

> 

> > +	if (cmd > ARRAY_SIZE(physdevop_len))

> > +		return -ENOSYS;

> >

> > +	len = physdevop_len[cmd];

> > +	memcpy(&op.u, arg, len);

> 

> You'll want an array_nospec() or whatever its called these days.  This

> code is SP1-leaky.

> 

> Userspace controls cmd and can retrieve len by timing how many adjacent

> cache lines were pulled in my memcpy().


Well, maybe it can read beyond the bounds of physdevop_len[].
I doubt that the memcpy() will pull in many cache lines so you
can probably only determine whether the value is 0..63, 64..127 or 128+
Not likely to be much use.

	David
Arnd Bergmann Feb. 5, 2018, 3:47 p.m. UTC | #4 
On Mon, Feb 5, 2018 at 4:14 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 05.02.18 at 16:03, <arnd@arndb.de> wrote:

>>  int xen_event_channel_op_compat(int cmd, void *arg)

>>  {

>> -     struct evtchn_op op;

>> +     struct evtchn_op op = { .cmd = cmd, };

>> +     size_t len;

>>       int rc;

>>

>> -     op.cmd = cmd;

>> -     memcpy(&op.u, arg, sizeof(op.u));

>> -     rc = _hypercall1(int, event_channel_op_compat, &op);

>> -

>> -     switch (cmd) {

>> -     case EVTCHNOP_close:

>> -     case EVTCHNOP_send:

>> -     case EVTCHNOP_bind_vcpu:

>> -     case EVTCHNOP_unmask:

>> -             /* no output */

>> -             break;

>> +     if (cmd > ARRAY_SIZE(evtchnop_len))

>> +             return -ENOSYS;

>

>>= perhaps?


Argh, of course. This is why I preferred the switch/case version, I knew
I'd screw this up somehow ;-)

       Arnd
Nicolas Pitre Feb. 5, 2018, 3:51 p.m. UTC | #5 
On Mon, 5 Feb 2018, Arnd Bergmann wrote:

> +	if (cmd > ARRAY_SIZE(evtchnop_len))

> +		return -ENOSYS;

> +	len = evtchnop_len[cmd];


What if cmd == ARRAY_SIZE(evtchnop_len) ?


Nicolas
Arnd Bergmann Feb. 9, 2018, 12:57 p.m. UTC | #6 
On Mon, Feb 5, 2018 at 4:14 PM, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> On 05/02/18 15:03, Arnd Bergmann wrote:

>

> Snipping deleted code to make things clearer:

>

>> +     if (cmd > ARRAY_SIZE(physdevop_len))

>> +             return -ENOSYS;

>>

>> +     len = physdevop_len[cmd];

>> +     memcpy(&op.u, arg, len);

>

> You'll want an array_nospec() or whatever its called these days.  This

> code is SP1-leaky.


Maybe the best solution would be to remove the file completely. From
looking at the Xen git history, we only need this to run on Xen 3.0.2
or earlier, those early Xen releases (according to Wikipedia) never
even supported running modern kernel versions anyway, so the code
appears to be completely pointless here.

However, aside from this driver, I wonder if we should be worried about
Spectre type 1 attacks on similar code, when gcc-8 turns a switch/case
statement into an array lookup behind our back, e.g. in an ioctl handler.
Has anybody got this on their radar?

       Arnd
David Laight Feb. 9, 2018, 2:13 p.m. UTC | #7 
From: Arnd Bergmann

> Sent: 09 February 2018 12:58

...
> However, aside from this driver, I wonder if we should be worried about

> Spectre type 1 attacks on similar code, when gcc-8 turns a switch/case

> statement into an array lookup behind our back, e.g. in an ioctl handler.

> Has anybody got this on their radar?


The canonical code for a switch statement is to jump indirect on an array
of code pointers.
ioctl handlers probably use a series of compares because the values are
sparse.

Also remember that gcc-8 will convert dense switch statements that just
load a value into a data array lookup.

I guess both those jump tables are potential attack vectors.
Not quite sure how they might be used to leak info though.

	David
Arnd Bergmann Feb. 9, 2018, 2:21 p.m. UTC | #8 
On Fri, Feb 9, 2018 at 3:13 PM, David Laight <David.Laight@aculab.com> wrote:
> From: Arnd Bergmann
>> Sent: 09 February 2018 12:58
> ...
>> However, aside from this driver, I wonder if we should be worried about
>> Spectre type 1 attacks on similar code, when gcc-8 turns a switch/case
>> statement into an array lookup behind our back, e.g. in an ioctl handler.
>> Has anybody got this on their radar?
>
> The canonical code for a switch statement is to jump indirect on an array
> of code pointers.
> ioctl handlers probably use a series of compares because the values are
> sparse.

The majority of ioctl handlers is sparse enough that a table lookup wouldn't
work, but there are still subsystems that never fully adopted the _IOC()
macros, e.g. tty or socket ioctls are just consecutive numbers.

> Also remember that gcc-8 will convert dense switch statements that just
> load a value into a data array lookup.

Right, that's the case I'm interested in here. I don't know how many of
those exist in the kernel, as this would again be a small subset of the
switch()/case statements that use consecutive numbers.

> I guess both those jump tables are potential attack vectors.
> Not quite sure how they might be used to leak info though.

When I tested the xen fallback code with gcc-7.3, I noticed a retpoline
getting generated for pointer array, so that should be safe.

      Arnd
Dan Williams Feb. 9, 2018, 4:20 p.m. UTC | #9 
On Fri, Feb 9, 2018 at 6:21 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Fri, Feb 9, 2018 at 3:13 PM, David Laight <David.Laight@aculab.com> wrote:

>> From: Arnd Bergmann

>>> Sent: 09 February 2018 12:58

>> ...

>>> However, aside from this driver, I wonder if we should be worried about

>>> Spectre type 1 attacks on similar code, when gcc-8 turns a switch/case

>>> statement into an array lookup behind our back, e.g. in an ioctl handler.

>>> Has anybody got this on their radar?

>>

>> The canonical code for a switch statement is to jump indirect on an array

>> of code pointers.

>> ioctl handlers probably use a series of compares because the values are

>> sparse.

>

> The majority of ioctl handlers is sparse enough that a table lookup wouldn't

> work, but there are still subsystems that never fully adopted the _IOC()

> macros, e.g. tty or socket ioctls are just consecutive numbers.

>

>> Also remember that gcc-8 will convert dense switch statements that just

>> load a value into a data array lookup.

>

> Right, that's the case I'm interested in here. I don't know how many of

> those exist in the kernel, as this would again be a small subset of the

> switch()/case statements that use consecutive numbers.

>

>> I guess both those jump tables are potential attack vectors.

>> Not quite sure how they might be used to leak info though.

>

> When I tested the xen fallback code with gcc-7.3, I noticed a retpoline

> getting generated for pointer array, so that should be safe.


The retpoline would protect the indirect call itself, but not the
lookup into the array. So this needs the same protection as the
syscall table where we sanitize the lookup index in addition to the
retpoline on the actual call.
diff mbox series

Patch

diff --git a/drivers/xen/fallback.c b/drivers/xen/fallback.c
index b04fb64c5a91..091d45fa4fe6 100644
--- a/drivers/xen/fallback.c
+++ b/drivers/xen/fallback.c
@@ -5,76 +5,60 @@ 
 #include <asm/hypervisor.h>
 #include <asm/xen/hypercall.h>
 
+static const size_t evtchnop_len[] = {
+	[EVTCHNOP_bind_interdomain] = sizeof(struct evtchn_bind_interdomain),
+	[EVTCHNOP_bind_virq]	    = sizeof(struct evtchn_bind_virq),
+	[EVTCHNOP_bind_pirq]	    = sizeof(struct evtchn_bind_pirq),
+	[EVTCHNOP_close]	    = sizeof(struct evtchn_close),
+	[EVTCHNOP_send]		    = sizeof(struct evtchn_send),
+	[EVTCHNOP_alloc_unbound]    = sizeof(struct evtchn_alloc_unbound),
+	[EVTCHNOP_bind_ipi]	    = sizeof(struct evtchn_bind_ipi),
+	[EVTCHNOP_status]	    = sizeof(struct evtchn_status),
+	[EVTCHNOP_bind_vcpu]	    = sizeof(struct evtchn_bind_vcpu),
+	[EVTCHNOP_unmask]	    = sizeof(struct evtchn_unmask),
+};
+
 int xen_event_channel_op_compat(int cmd, void *arg)
 {
-	struct evtchn_op op;
+	struct evtchn_op op = { .cmd = cmd, };
+	size_t len;
 	int rc;
 
-	op.cmd = cmd;
-	memcpy(&op.u, arg, sizeof(op.u));
-	rc = _hypercall1(int, event_channel_op_compat, &op);
-
-	switch (cmd) {
-	case EVTCHNOP_close:
-	case EVTCHNOP_send:
-	case EVTCHNOP_bind_vcpu:
-	case EVTCHNOP_unmask:
-		/* no output */
-		break;
+	if (cmd > ARRAY_SIZE(evtchnop_len))
+		return -ENOSYS;
 
-#define COPY_BACK(eop) \
-	case EVTCHNOP_##eop: \
-		memcpy(arg, &op.u.eop, sizeof(op.u.eop)); \
-		break
-
-	COPY_BACK(bind_interdomain);
-	COPY_BACK(bind_virq);
-	COPY_BACK(bind_pirq);
-	COPY_BACK(status);
-	COPY_BACK(alloc_unbound);
-	COPY_BACK(bind_ipi);
-#undef COPY_BACK
-
-	default:
-		WARN_ON(rc != -ENOSYS);
-		break;
-	}
+	len = evtchnop_len[cmd];
+	memcpy(&op.u, arg, len);
+	rc = _hypercall1(int, event_channel_op_compat, &op);
+	memcpy(arg, &op.u, len);
 
 	return rc;
 }
 EXPORT_SYMBOL_GPL(xen_event_channel_op_compat);
 
+static const size_t physdevop_len[] = {
+	[PHYSDEVOP_IRQ_UNMASK_NOTIFY] = 0,
+	[PHYSDEVOP_irq_status_query]  = sizeof(struct physdev_irq_status_query),
+	[PHYSDEVOP_set_iopl]	      = sizeof(struct physdev_set_iopl),
+	[PHYSDEVOP_set_iobitmap]      = sizeof(struct physdev_set_iobitmap),
+	[PHYSDEVOP_apic_read]	      = sizeof(struct physdev_apic),
+	[PHYSDEVOP_apic_write]	      = sizeof(struct physdev_apic),
+	[PHYSDEVOP_ASSIGN_VECTOR]     = sizeof(struct physdev_irq),
+};
+
 int xen_physdev_op_compat(int cmd, void *arg)
 {
-	struct physdev_op op;
+	struct physdev_op op = { .cmd = cmd, };
+	size_t len;
 	int rc;
 
-	op.cmd = cmd;
-	memcpy(&op.u, arg, sizeof(op.u));
-	rc = _hypercall1(int, physdev_op_compat, &op);
-
-	switch (cmd) {
-	case PHYSDEVOP_IRQ_UNMASK_NOTIFY:
-	case PHYSDEVOP_set_iopl:
-	case PHYSDEVOP_set_iobitmap:
-	case PHYSDEVOP_apic_write:
-		/* no output */
-		break;
+	if (cmd > ARRAY_SIZE(physdevop_len))
+		return -ENOSYS;
 
-#define COPY_BACK(pop, fld) \
-	case PHYSDEVOP_##pop: \
-		memcpy(arg, &op.u.fld, sizeof(op.u.fld)); \
-		break
-
-	COPY_BACK(irq_status_query, irq_status_query);
-	COPY_BACK(apic_read, apic_op);
-	COPY_BACK(ASSIGN_VECTOR, irq_op);
-#undef COPY_BACK
-
-	default:
-		WARN_ON(rc != -ENOSYS);
-		break;
-	}
+	len = physdevop_len[cmd];
+	memcpy(&op.u, arg, len);
+	rc = _hypercall1(int, physdev_op_compat, &op);
+	memcpy(arg, &op.u, len);
 
 	return rc;
 }