* [PATCH] [v2] xen: hypercall: fix out-of-bounds memcpy
@ 2018-02-05 15:03 ` Arnd Bergmann
0 siblings, 0 replies; 20+ messages in thread
From: Arnd Bergmann @ 2018-02-05 15:03 UTC (permalink / raw)
To: Boris Ostrovsky, Juergen Gross
Cc: David Laight, Dan Carpenter, Arnd Bergmann, xen-devel, linux-kernel
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(-)
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;
}
--
2.9.0
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH] [v2] xen: hypercall: fix out-of-bounds memcpy
@ 2018-02-05 15:03 ` Arnd Bergmann
0 siblings, 0 replies; 20+ messages in thread
From: Arnd Bergmann @ 2018-02-05 15:03 UTC (permalink / raw)
To: Boris Ostrovsky, Juergen Gross
Cc: xen-devel, David Laight, Arnd Bergmann, Dan Carpenter, linux-kernel
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(-)
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;
}
--
2.9.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [Xen-devel] [PATCH] [v2] xen: hypercall: fix out-of-bounds memcpy
2018-02-05 15:03 ` Arnd Bergmann
(?)
(?)
@ 2018-02-05 15:14 ` Andrew Cooper
2018-02-05 15:28 ` David Laight
` (3 more replies)
-1 siblings, 4 replies; 20+ messages in thread
From: Andrew Cooper @ 2018-02-05 15:14 UTC (permalink / raw)
To: Arnd Bergmann, Boris Ostrovsky, Juergen Gross
Cc: xen-devel, David Laight, Dan Carpenter, linux-kernel
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] [v2] xen: hypercall: fix out-of-bounds memcpy
2018-02-05 15:03 ` Arnd Bergmann
(?)
@ 2018-02-05 15:14 ` Andrew Cooper
-1 siblings, 0 replies; 20+ messages in thread
From: Andrew Cooper @ 2018-02-05 15:14 UTC (permalink / raw)
To: Arnd Bergmann, Boris Ostrovsky, Juergen Gross
Cc: xen-devel, David Laight, linux-kernel, Dan Carpenter
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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Xen-devel] [PATCH] [v2] xen: hypercall: fix out-of-bounds memcpy
2018-02-05 15:03 ` Arnd Bergmann
` (3 preceding siblings ...)
(?)
@ 2018-02-05 15:14 ` Jan Beulich
2018-02-05 15:47 ` Arnd Bergmann
2018-02-05 15:47 ` [Xen-devel] " Arnd Bergmann
-1 siblings, 2 replies; 20+ messages in thread
From: Jan Beulich @ 2018-02-05 15:14 UTC (permalink / raw)
To: Arnd Bergmann
Cc: David Laight, xen-devel, Boris Ostrovsky, Dan Carpenter,
Juergen Gross, linux-kernel
>>> 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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] [v2] xen: hypercall: fix out-of-bounds memcpy
2018-02-05 15:03 ` Arnd Bergmann
` (2 preceding siblings ...)
(?)
@ 2018-02-05 15:14 ` Jan Beulich
-1 siblings, 0 replies; 20+ messages in thread
From: Jan Beulich @ 2018-02-05 15:14 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Juergen Gross, linux-kernel, David Laight, xen-devel,
Boris Ostrovsky, Dan Carpenter
>>> 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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 20+ messages in thread
* RE: [Xen-devel] [PATCH] [v2] xen: hypercall: fix out-of-bounds memcpy
2018-02-05 15:14 ` [Xen-devel] " Andrew Cooper
@ 2018-02-05 15:28 ` David Laight
2018-02-05 15:28 ` David Laight
` (2 subsequent siblings)
3 siblings, 0 replies; 20+ messages in thread
From: David Laight @ 2018-02-05 15:28 UTC (permalink / raw)
To: 'Andrew Cooper', Arnd Bergmann, Boris Ostrovsky, Juergen Gross
Cc: xen-devel, Dan Carpenter, linux-kernel
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] [v2] xen: hypercall: fix out-of-bounds memcpy
2018-02-05 15:14 ` [Xen-devel] " Andrew Cooper
2018-02-05 15:28 ` David Laight
@ 2018-02-05 15:28 ` David Laight
2018-02-09 12:57 ` [Xen-devel] " Arnd Bergmann
2018-02-09 12:57 ` Arnd Bergmann
3 siblings, 0 replies; 20+ messages in thread
From: David Laight @ 2018-02-05 15:28 UTC (permalink / raw)
To: 'Andrew Cooper', Arnd Bergmann, Boris Ostrovsky, Juergen Gross
Cc: xen-devel, linux-kernel, Dan Carpenter
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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Xen-devel] [PATCH] [v2] xen: hypercall: fix out-of-bounds memcpy
2018-02-05 15:14 ` [Xen-devel] " Jan Beulich
2018-02-05 15:47 ` Arnd Bergmann
@ 2018-02-05 15:47 ` Arnd Bergmann
1 sibling, 0 replies; 20+ messages in thread
From: Arnd Bergmann @ 2018-02-05 15:47 UTC (permalink / raw)
To: Jan Beulich
Cc: David Laight, xen-devel, Boris Ostrovsky, Dan Carpenter,
Juergen Gross, Linux Kernel Mailing List
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] [v2] xen: hypercall: fix out-of-bounds memcpy
2018-02-05 15:14 ` [Xen-devel] " Jan Beulich
@ 2018-02-05 15:47 ` Arnd Bergmann
2018-02-05 15:47 ` [Xen-devel] " Arnd Bergmann
1 sibling, 0 replies; 20+ messages in thread
From: Arnd Bergmann @ 2018-02-05 15:47 UTC (permalink / raw)
To: Jan Beulich
Cc: Juergen Gross, Linux Kernel Mailing List, David Laight,
xen-devel, Boris Ostrovsky, Dan Carpenter
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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] [v2] xen: hypercall: fix out-of-bounds memcpy
2018-02-05 15:03 ` Arnd Bergmann
` (5 preceding siblings ...)
(?)
@ 2018-02-05 15:51 ` Nicolas Pitre
-1 siblings, 0 replies; 20+ messages in thread
From: Nicolas Pitre @ 2018-02-05 15:51 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Boris Ostrovsky, Juergen Gross, David Laight, Dan Carpenter,
xen-devel, linux-kernel
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] [v2] xen: hypercall: fix out-of-bounds memcpy
2018-02-05 15:03 ` Arnd Bergmann
` (4 preceding siblings ...)
(?)
@ 2018-02-05 15:51 ` Nicolas Pitre
-1 siblings, 0 replies; 20+ messages in thread
From: Nicolas Pitre @ 2018-02-05 15:51 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Juergen Gross, linux-kernel, David Laight, xen-devel,
Boris Ostrovsky, Dan Carpenter
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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Xen-devel] [PATCH] [v2] xen: hypercall: fix out-of-bounds memcpy
2018-02-05 15:14 ` [Xen-devel] " Andrew Cooper
2018-02-05 15:28 ` David Laight
2018-02-05 15:28 ` David Laight
@ 2018-02-09 12:57 ` Arnd Bergmann
2018-02-09 14:13 ` David Laight
2018-02-09 14:13 ` [Xen-devel] " David Laight
2018-02-09 12:57 ` Arnd Bergmann
3 siblings, 2 replies; 20+ messages in thread
From: Arnd Bergmann @ 2018-02-09 12:57 UTC (permalink / raw)
To: Andrew Cooper
Cc: Boris Ostrovsky, Juergen Gross, xen-devel, David Laight,
Dan Carpenter, Linux Kernel Mailing List, Kees Cook,
Dan Williams, David Woodhouse
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] [v2] xen: hypercall: fix out-of-bounds memcpy
2018-02-05 15:14 ` [Xen-devel] " Andrew Cooper
` (2 preceding siblings ...)
2018-02-09 12:57 ` [Xen-devel] " Arnd Bergmann
@ 2018-02-09 12:57 ` Arnd Bergmann
3 siblings, 0 replies; 20+ messages in thread
From: Arnd Bergmann @ 2018-02-09 12:57 UTC (permalink / raw)
To: Andrew Cooper
Cc: Juergen Gross, Kees Cook, Linux Kernel Mailing List,
David Laight, David Woodhouse, xen-devel, Boris Ostrovsky,
Dan Williams, Dan Carpenter
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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 20+ messages in thread
* RE: [Xen-devel] [PATCH] [v2] xen: hypercall: fix out-of-bounds memcpy
2018-02-09 12:57 ` [Xen-devel] " Arnd Bergmann
2018-02-09 14:13 ` David Laight
@ 2018-02-09 14:13 ` David Laight
2018-02-09 14:21 ` Arnd Bergmann
1 sibling, 1 reply; 20+ messages in thread
From: David Laight @ 2018-02-09 14:13 UTC (permalink / raw)
To: 'Arnd Bergmann', Andrew Cooper
Cc: Boris Ostrovsky, Juergen Gross, xen-devel, Dan Carpenter,
Linux Kernel Mailing List, Kees Cook, Dan Williams,
David Woodhouse
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] [v2] xen: hypercall: fix out-of-bounds memcpy
2018-02-09 12:57 ` [Xen-devel] " Arnd Bergmann
@ 2018-02-09 14:13 ` David Laight
2018-02-09 14:13 ` [Xen-devel] " David Laight
1 sibling, 0 replies; 20+ messages in thread
From: David Laight @ 2018-02-09 14:13 UTC (permalink / raw)
To: 'Arnd Bergmann', Andrew Cooper
Cc: Juergen Gross, Kees Cook, Linux Kernel Mailing List,
David Woodhouse, xen-devel, Boris Ostrovsky, Dan Williams,
Dan Carpenter
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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Xen-devel] [PATCH] [v2] xen: hypercall: fix out-of-bounds memcpy
2018-02-09 14:13 ` [Xen-devel] " David Laight
@ 2018-02-09 14:21 ` Arnd Bergmann
0 siblings, 0 replies; 20+ messages in thread
From: Arnd Bergmann @ 2018-02-09 14:21 UTC (permalink / raw)
To: David Laight
Cc: Andrew Cooper, Boris Ostrovsky, Juergen Gross, xen-devel,
Dan Carpenter, Linux Kernel Mailing List, Kees Cook,
Dan Williams, David Woodhouse
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] [v2] xen: hypercall: fix out-of-bounds memcpy
@ 2018-02-09 14:21 ` Arnd Bergmann
0 siblings, 0 replies; 20+ messages in thread
From: Arnd Bergmann @ 2018-02-09 14:21 UTC (permalink / raw)
To: David Laight
Cc: Juergen Gross, Kees Cook, Andrew Cooper,
Linux Kernel Mailing List, David Woodhouse, xen-devel,
Boris Ostrovsky, Dan Williams, Dan Carpenter
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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Xen-devel] [PATCH] [v2] xen: hypercall: fix out-of-bounds memcpy
2018-02-09 14:21 ` Arnd Bergmann
(?)
@ 2018-02-09 16:20 ` Dan Williams
-1 siblings, 0 replies; 20+ messages in thread
From: Dan Williams @ 2018-02-09 16:20 UTC (permalink / raw)
To: Arnd Bergmann
Cc: David Laight, Andrew Cooper, Boris Ostrovsky, Juergen Gross,
xen-devel, Dan Carpenter, Linux Kernel Mailing List, Kees Cook,
David Woodhouse
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.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] [v2] xen: hypercall: fix out-of-bounds memcpy
2018-02-09 14:21 ` Arnd Bergmann
(?)
(?)
@ 2018-02-09 16:20 ` Dan Williams
-1 siblings, 0 replies; 20+ messages in thread
From: Dan Williams @ 2018-02-09 16:20 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Juergen Gross, Kees Cook, Andrew Cooper,
Linux Kernel Mailing List, David Laight, David Woodhouse,
xen-devel, Boris Ostrovsky, Dan Carpenter
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.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2018-02-09 16:20 UTC | newest]
Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-02-05 15:03 [PATCH] [v2] xen: hypercall: fix out-of-bounds memcpy Arnd Bergmann
2018-02-05 15:03 ` Arnd Bergmann
2018-02-05 15:14 ` Andrew Cooper
2018-02-05 15:14 ` [Xen-devel] " Andrew Cooper
2018-02-05 15:28 ` David Laight
2018-02-05 15:28 ` David Laight
2018-02-09 12:57 ` [Xen-devel] " Arnd Bergmann
2018-02-09 14:13 ` David Laight
2018-02-09 14:13 ` [Xen-devel] " David Laight
2018-02-09 14:21 ` Arnd Bergmann
2018-02-09 14:21 ` Arnd Bergmann
2018-02-09 16:20 ` [Xen-devel] " Dan Williams
2018-02-09 16:20 ` Dan Williams
2018-02-09 12:57 ` Arnd Bergmann
2018-02-05 15:14 ` Jan Beulich
2018-02-05 15:14 ` [Xen-devel] " Jan Beulich
2018-02-05 15:47 ` Arnd Bergmann
2018-02-05 15:47 ` [Xen-devel] " Arnd Bergmann
2018-02-05 15:51 ` Nicolas Pitre
2018-02-05 15:51 ` Nicolas Pitre
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.