* [PATCH] xen: hypercall: fix out-of-bounds memcpy
@ 2018-02-02 15:32 Arnd Bergmann
0 siblings, 0 replies; 26+ messages in thread
From: Arnd Bergmann @ 2018-02-02 15:32 UTC (permalink / raw)
To: Boris Ostrovsky, Juergen Gross
Cc: Andi Kleen, Arnd Bergmann, Nicolas Pitre, linux-kernel,
Jan Beulich, xen-devel, Dan Carpenter
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>
---
drivers/xen/fallback.c | 94 ++++++++++++++++++++++++++++----------------------
1 file changed, 53 insertions(+), 41 deletions(-)
diff --git a/drivers/xen/fallback.c b/drivers/xen/fallback.c
index b04fb64c5a91..eded8dd821ad 100644
--- a/drivers/xen/fallback.c
+++ b/drivers/xen/fallback.c
@@ -7,75 +7,87 @@
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_bind_interdomain:
+ len = sizeof(struct evtchn_bind_interdomain);
+ break;
+ case EVTCHNOP_bind_virq:
+ len = sizeof(struct evtchn_bind_virq);
+ break;
+ case EVTCHNOP_bind_pirq:
+ len = sizeof(struct evtchn_bind_pirq);
+ break;
case EVTCHNOP_close:
+ len = sizeof(struct evtchn_close);
+ break;
case EVTCHNOP_send:
+ len = sizeof(struct evtchn_send);
+ break;
+ case EVTCHNOP_alloc_unbound:
+ len = sizeof(struct evtchn_alloc_unbound);
+ break;
+ case EVTCHNOP_bind_ipi:
+ len = sizeof(struct evtchn_bind_ipi);
+ break;
+ case EVTCHNOP_status:
+ len = sizeof(struct evtchn_status);
+ break;
case EVTCHNOP_bind_vcpu:
+ len = sizeof(struct evtchn_bind_vcpu);
+ break;
case EVTCHNOP_unmask:
- /* no output */
+ len = sizeof(struct evtchn_unmask);
break;
-
-#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;
+ return -ENOSYS;
}
+ 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);
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:
+ len = 0;
+ break;
+ case PHYSDEVOP_irq_status_query:
+ len = sizeof(struct physdev_irq_status_query);
+ break;
case PHYSDEVOP_set_iopl:
+ len = sizeof(struct physdev_set_iopl);
+ break;
case PHYSDEVOP_set_iobitmap:
+ len = sizeof(struct physdev_set_iobitmap);
+ break;
+ case PHYSDEVOP_apic_read:
case PHYSDEVOP_apic_write:
- /* no output */
+ len = sizeof(struct physdev_apic);
break;
-
-#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);
+ case PHYSDEVOP_ASSIGN_VECTOR:
+ len = sizeof(struct physdev_irq);
break;
+ default:
+ return -ENOSYS;
}
+ memcpy(&op.u, arg, len);
+ rc = _hypercall1(int, physdev_op_compat, &op);
+ memcpy(arg, &op.u, len);
+
return rc;
}
EXPORT_SYMBOL_GPL(xen_physdev_op_compat);
--
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] 26+ messages in thread* [PATCH] xen: hypercall: fix out-of-bounds memcpy
@ 2018-02-02 15:32 Arnd Bergmann
2018-02-02 15:53 ` Dan Carpenter
` (3 more replies)
0 siblings, 4 replies; 26+ messages in thread
From: Arnd Bergmann @ 2018-02-02 15:32 UTC (permalink / raw)
To: Boris Ostrovsky, Juergen Gross
Cc: Nicolas Pitre, Andi Kleen, Dan Carpenter, Jan Beulich,
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>
---
drivers/xen/fallback.c | 94 ++++++++++++++++++++++++++++----------------------
1 file changed, 53 insertions(+), 41 deletions(-)
diff --git a/drivers/xen/fallback.c b/drivers/xen/fallback.c
index b04fb64c5a91..eded8dd821ad 100644
--- a/drivers/xen/fallback.c
+++ b/drivers/xen/fallback.c
@@ -7,75 +7,87 @@
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_bind_interdomain:
+ len = sizeof(struct evtchn_bind_interdomain);
+ break;
+ case EVTCHNOP_bind_virq:
+ len = sizeof(struct evtchn_bind_virq);
+ break;
+ case EVTCHNOP_bind_pirq:
+ len = sizeof(struct evtchn_bind_pirq);
+ break;
case EVTCHNOP_close:
+ len = sizeof(struct evtchn_close);
+ break;
case EVTCHNOP_send:
+ len = sizeof(struct evtchn_send);
+ break;
+ case EVTCHNOP_alloc_unbound:
+ len = sizeof(struct evtchn_alloc_unbound);
+ break;
+ case EVTCHNOP_bind_ipi:
+ len = sizeof(struct evtchn_bind_ipi);
+ break;
+ case EVTCHNOP_status:
+ len = sizeof(struct evtchn_status);
+ break;
case EVTCHNOP_bind_vcpu:
+ len = sizeof(struct evtchn_bind_vcpu);
+ break;
case EVTCHNOP_unmask:
- /* no output */
+ len = sizeof(struct evtchn_unmask);
break;
-
-#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;
+ return -ENOSYS;
}
+ 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);
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:
+ len = 0;
+ break;
+ case PHYSDEVOP_irq_status_query:
+ len = sizeof(struct physdev_irq_status_query);
+ break;
case PHYSDEVOP_set_iopl:
+ len = sizeof(struct physdev_set_iopl);
+ break;
case PHYSDEVOP_set_iobitmap:
+ len = sizeof(struct physdev_set_iobitmap);
+ break;
+ case PHYSDEVOP_apic_read:
case PHYSDEVOP_apic_write:
- /* no output */
+ len = sizeof(struct physdev_apic);
break;
-
-#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);
+ case PHYSDEVOP_ASSIGN_VECTOR:
+ len = sizeof(struct physdev_irq);
break;
+ default:
+ return -ENOSYS;
}
+ memcpy(&op.u, arg, len);
+ rc = _hypercall1(int, physdev_op_compat, &op);
+ memcpy(arg, &op.u, len);
+
return rc;
}
EXPORT_SYMBOL_GPL(xen_physdev_op_compat);
--
2.9.0
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH] xen: hypercall: fix out-of-bounds memcpy
2018-02-02 15:32 Arnd Bergmann
@ 2018-02-02 15:53 ` Dan Carpenter
2018-02-02 15:53 ` Dan Carpenter
` (2 subsequent siblings)
3 siblings, 0 replies; 26+ messages in thread
From: Dan Carpenter @ 2018-02-02 15:53 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Juergen Gross, Andi Kleen, Nicolas Pitre, linux-kernel,
Jan Beulich, xen-devel, Boris Ostrovsky
On Fri, Feb 02, 2018 at 04:32:31PM +0100, Arnd Bergmann wrote:
> 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
>
It was a more naive era. :P
> 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>
> ---
> drivers/xen/fallback.c | 94 ++++++++++++++++++++++++++++----------------------
> 1 file changed, 53 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/xen/fallback.c b/drivers/xen/fallback.c
> index b04fb64c5a91..eded8dd821ad 100644
> --- a/drivers/xen/fallback.c
> +++ b/drivers/xen/fallback.c
> @@ -7,75 +7,87 @@
>
> 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_bind_interdomain:
> + len = sizeof(struct evtchn_bind_interdomain);
> + break;
This was in the original code, but I'm slightly surpprised that we're
using a switch statement here instead of a table. I would have thought
this is a fast path but I don't know xen at all.
regards,
dan carpenter
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH] xen: hypercall: fix out-of-bounds memcpy
2018-02-02 15:32 Arnd Bergmann
2018-02-02 15:53 ` Dan Carpenter
@ 2018-02-02 15:53 ` Dan Carpenter
2018-02-02 16:11 ` Arnd Bergmann
2018-02-02 16:11 ` Arnd Bergmann
2018-02-02 23:33 ` Boris Ostrovsky
2018-02-02 23:33 ` Boris Ostrovsky
3 siblings, 2 replies; 26+ messages in thread
From: Dan Carpenter @ 2018-02-02 15:53 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Boris Ostrovsky, Juergen Gross, Nicolas Pitre, Andi Kleen,
Jan Beulich, xen-devel, linux-kernel
On Fri, Feb 02, 2018 at 04:32:31PM +0100, Arnd Bergmann wrote:
> 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
>
It was a more naive era. :P
> 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>
> ---
> drivers/xen/fallback.c | 94 ++++++++++++++++++++++++++++----------------------
> 1 file changed, 53 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/xen/fallback.c b/drivers/xen/fallback.c
> index b04fb64c5a91..eded8dd821ad 100644
> --- a/drivers/xen/fallback.c
> +++ b/drivers/xen/fallback.c
> @@ -7,75 +7,87 @@
>
> 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_bind_interdomain:
> + len = sizeof(struct evtchn_bind_interdomain);
> + break;
This was in the original code, but I'm slightly surpprised that we're
using a switch statement here instead of a table. I would have thought
this is a fast path but I don't know xen at all.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH] xen: hypercall: fix out-of-bounds memcpy
2018-02-02 15:53 ` Dan Carpenter
@ 2018-02-02 16:11 ` Arnd Bergmann
2018-02-02 16:34 ` Dan Carpenter
2018-02-02 16:34 ` Dan Carpenter
2018-02-02 16:11 ` Arnd Bergmann
1 sibling, 2 replies; 26+ messages in thread
From: Arnd Bergmann @ 2018-02-02 16:11 UTC (permalink / raw)
To: Dan Carpenter
Cc: Boris Ostrovsky, Juergen Gross, Nicolas Pitre, Andi Kleen,
Jan Beulich, xen-devel, Linux Kernel Mailing List
On Fri, Feb 2, 2018 at 4:53 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> On Fri, Feb 02, 2018 at 04:32:31PM +0100, Arnd Bergmann wrote:
>> --- a/drivers/xen/fallback.c
>> +++ b/drivers/xen/fallback.c
>> @@ -7,75 +7,87 @@
>>
>> 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_bind_interdomain:
>> + len = sizeof(struct evtchn_bind_interdomain);
>> + break;
>
> This was in the original code, but I'm slightly surpprised that we're
> using a switch statement here instead of a table. I would have thought
> this is a fast path but I don't know xen at all.
I thought about using a table, but figured the switch statement
had a lower risk of getting something slightly wrong during the
conversion.
I would expect gcc to turn this into a table lookup, since all the
constants are consecutive, but it should not really matter since
this is only the fallback path for ancient Xen releases. When Xen
guest support was first merged in 2007, it was already
deprecated.
Arnd
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH] xen: hypercall: fix out-of-bounds memcpy
2018-02-02 16:11 ` Arnd Bergmann
@ 2018-02-02 16:34 ` Dan Carpenter
2018-02-02 16:34 ` Dan Carpenter
1 sibling, 0 replies; 26+ messages in thread
From: Dan Carpenter @ 2018-02-02 16:34 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Boris Ostrovsky, Juergen Gross, Nicolas Pitre, Andi Kleen,
Jan Beulich, xen-devel, Linux Kernel Mailing List
On Fri, Feb 02, 2018 at 05:11:02PM +0100, Arnd Bergmann wrote:
> On Fri, Feb 2, 2018 at 4:53 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > On Fri, Feb 02, 2018 at 04:32:31PM +0100, Arnd Bergmann wrote:
> >> switch (cmd) {
> >> + case EVTCHNOP_bind_interdomain:
> >> + len = sizeof(struct evtchn_bind_interdomain);
> >> + break;
> >
> > This was in the original code, but I'm slightly surpprised that we're
> > using a switch statement here instead of a table. I would have thought
> > this is a fast path but I don't know xen at all.
>
> I thought about using a table, but figured the switch statement
> had a lower risk of getting something slightly wrong during the
> conversion.
>
> I would expect gcc to turn this into a table lookup, since all the
> constants are consecutive, but it should not really matter since
> this is only the fallback path for ancient Xen releases. When Xen
> guest support was first merged in 2007, it was already
> deprecated.
>
Ah. Ok. That makes sense.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH] xen: hypercall: fix out-of-bounds memcpy
2018-02-02 16:11 ` Arnd Bergmann
2018-02-02 16:34 ` Dan Carpenter
@ 2018-02-02 16:34 ` Dan Carpenter
1 sibling, 0 replies; 26+ messages in thread
From: Dan Carpenter @ 2018-02-02 16:34 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Juergen Gross, Andi Kleen, Nicolas Pitre,
Linux Kernel Mailing List, Jan Beulich, xen-devel,
Boris Ostrovsky
On Fri, Feb 02, 2018 at 05:11:02PM +0100, Arnd Bergmann wrote:
> On Fri, Feb 2, 2018 at 4:53 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > On Fri, Feb 02, 2018 at 04:32:31PM +0100, Arnd Bergmann wrote:
> >> switch (cmd) {
> >> + case EVTCHNOP_bind_interdomain:
> >> + len = sizeof(struct evtchn_bind_interdomain);
> >> + break;
> >
> > This was in the original code, but I'm slightly surpprised that we're
> > using a switch statement here instead of a table. I would have thought
> > this is a fast path but I don't know xen at all.
>
> I thought about using a table, but figured the switch statement
> had a lower risk of getting something slightly wrong during the
> conversion.
>
> I would expect gcc to turn this into a table lookup, since all the
> constants are consecutive, but it should not really matter since
> this is only the fallback path for ancient Xen releases. When Xen
> guest support was first merged in 2007, it was already
> deprecated.
>
Ah. Ok. That makes sense.
regards,
dan carpenter
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] xen: hypercall: fix out-of-bounds memcpy
2018-02-02 15:53 ` Dan Carpenter
2018-02-02 16:11 ` Arnd Bergmann
@ 2018-02-02 16:11 ` Arnd Bergmann
1 sibling, 0 replies; 26+ messages in thread
From: Arnd Bergmann @ 2018-02-02 16:11 UTC (permalink / raw)
To: Dan Carpenter
Cc: Juergen Gross, Andi Kleen, Nicolas Pitre,
Linux Kernel Mailing List, Jan Beulich, xen-devel,
Boris Ostrovsky
On Fri, Feb 2, 2018 at 4:53 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> On Fri, Feb 02, 2018 at 04:32:31PM +0100, Arnd Bergmann wrote:
>> --- a/drivers/xen/fallback.c
>> +++ b/drivers/xen/fallback.c
>> @@ -7,75 +7,87 @@
>>
>> 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_bind_interdomain:
>> + len = sizeof(struct evtchn_bind_interdomain);
>> + break;
>
> This was in the original code, but I'm slightly surpprised that we're
> using a switch statement here instead of a table. I would have thought
> this is a fast path but I don't know xen at all.
I thought about using a table, but figured the switch statement
had a lower risk of getting something slightly wrong during the
conversion.
I would expect gcc to turn this into a table lookup, since all the
constants are consecutive, but it should not really matter since
this is only the fallback path for ancient Xen releases. When Xen
guest support was first merged in 2007, it was already
deprecated.
Arnd
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] xen: hypercall: fix out-of-bounds memcpy
2018-02-02 15:32 Arnd Bergmann
2018-02-02 15:53 ` Dan Carpenter
2018-02-02 15:53 ` Dan Carpenter
@ 2018-02-02 23:33 ` Boris Ostrovsky
2018-02-03 15:12 ` Arnd Bergmann
` (3 more replies)
2018-02-02 23:33 ` Boris Ostrovsky
3 siblings, 4 replies; 26+ messages in thread
From: Boris Ostrovsky @ 2018-02-02 23:33 UTC (permalink / raw)
To: Arnd Bergmann, Juergen Gross
Cc: Nicolas Pitre, Andi Kleen, Dan Carpenter, Jan Beulich, xen-devel,
linux-kernel
On 02/02/2018 10:32 AM, Arnd Bergmann wrote:
> 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>
> ---
> drivers/xen/fallback.c | 94 ++++++++++++++++++++++++++++----------------------
> 1 file changed, 53 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/xen/fallback.c b/drivers/xen/fallback.c
> index b04fb64c5a91..eded8dd821ad 100644
> --- a/drivers/xen/fallback.c
> +++ b/drivers/xen/fallback.c
> @@ -7,75 +7,87 @@
>
> 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_bind_interdomain:
> + len = sizeof(struct evtchn_bind_interdomain);
> + break;
> + case EVTCHNOP_bind_virq:
> + len = sizeof(struct evtchn_bind_virq);
> + break;
> + case EVTCHNOP_bind_pirq:
> + len = sizeof(struct evtchn_bind_pirq);
> + break;
> case EVTCHNOP_close:
> + len = sizeof(struct evtchn_close);
> + break;
> case EVTCHNOP_send:
> + len = sizeof(struct evtchn_send);
> + break;
> + case EVTCHNOP_alloc_unbound:
> + len = sizeof(struct evtchn_alloc_unbound);
> + break;
> + case EVTCHNOP_bind_ipi:
> + len = sizeof(struct evtchn_bind_ipi);
> + break;
> + case EVTCHNOP_status:
> + len = sizeof(struct evtchn_status);
> + break;
> case EVTCHNOP_bind_vcpu:
> + len = sizeof(struct evtchn_bind_vcpu);
> + break;
> case EVTCHNOP_unmask:
> - /* no output */
> + len = sizeof(struct evtchn_unmask);
> break;
> -
> -#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;
> + return -ENOSYS;
> }
>
> + memcpy(&op.u, arg, len);
> + rc = _hypercall1(int, event_channel_op_compat, &op);
> + memcpy(arg, &op.u, len);
We don't copy back for all commands, only those that are COPY_BACK.
> +
> return rc;
> }
> EXPORT_SYMBOL_GPL(xen_event_channel_op_compat);
>
> 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:
> + len = 0;
> + break;
> + case PHYSDEVOP_irq_status_query:
> + len = sizeof(struct physdev_irq_status_query);
> + break;
> case PHYSDEVOP_set_iopl:
> + len = sizeof(struct physdev_set_iopl);
> + break;
> case PHYSDEVOP_set_iobitmap:
> + len = sizeof(struct physdev_set_iobitmap);
> + break;
> + case PHYSDEVOP_apic_read:
> case PHYSDEVOP_apic_write:
> - /* no output */
> + len = sizeof(struct physdev_apic);
> break;
> -
> -#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);
> + case PHYSDEVOP_ASSIGN_VECTOR:
> + len = sizeof(struct physdev_irq);
> break;
> + default:
> + return -ENOSYS;
> }
>
> + memcpy(&op.u, arg, len);
> + rc = _hypercall1(int, physdev_op_compat, &op);
> + memcpy(arg, &op.u, len);
And the same is true here.
-boris
> +
> return rc;
> }
> EXPORT_SYMBOL_GPL(xen_physdev_op_compat);
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH] xen: hypercall: fix out-of-bounds memcpy
2018-02-02 23:33 ` Boris Ostrovsky
@ 2018-02-03 15:12 ` Arnd Bergmann
2018-02-03 15:12 ` Arnd Bergmann
` (2 subsequent siblings)
3 siblings, 0 replies; 26+ messages in thread
From: Arnd Bergmann @ 2018-02-03 15:12 UTC (permalink / raw)
To: Boris Ostrovsky
Cc: Juergen Gross, Andi Kleen, Nicolas Pitre,
Linux Kernel Mailing List, Jan Beulich, xen-devel, Dan Carpenter
On Sat, Feb 3, 2018 at 12:33 AM, Boris Ostrovsky
<boris.ostrovsky@oracle.com> wrote:
> On 02/02/2018 10:32 AM, Arnd Bergmann wrote:
>> 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>
>> ---
>> drivers/xen/fallback.c | 94 ++++++++++++++++++++++++++++----------------------
>> 1 file changed, 53 insertions(+), 41 deletions(-)
>>
>> default:
>> - WARN_ON(rc != -ENOSYS);
>> - break;
>> + return -ENOSYS;
>> }
>>
>> + memcpy(&op.u, arg, len);
>> + rc = _hypercall1(int, event_channel_op_compat, &op);
>> + memcpy(arg, &op.u, len);
>
>
> We don't copy back for all commands, only those that are COPY_BACK.
Not sure what you mean. Is it harmful to copy back the data for the others
in any way? Otherwise I wouldn't micro-optimize this.
Arnd
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH] xen: hypercall: fix out-of-bounds memcpy
2018-02-02 23:33 ` Boris Ostrovsky
2018-02-03 15:12 ` Arnd Bergmann
@ 2018-02-03 15:12 ` Arnd Bergmann
2018-02-03 17:08 ` Boris Ostrovsky
2018-02-03 17:08 ` Boris Ostrovsky
2018-02-05 12:11 ` David Laight
2018-02-05 12:11 ` David Laight
3 siblings, 2 replies; 26+ messages in thread
From: Arnd Bergmann @ 2018-02-03 15:12 UTC (permalink / raw)
To: Boris Ostrovsky
Cc: Juergen Gross, Nicolas Pitre, Andi Kleen, Dan Carpenter,
Jan Beulich, xen-devel, Linux Kernel Mailing List
On Sat, Feb 3, 2018 at 12:33 AM, Boris Ostrovsky
<boris.ostrovsky@oracle.com> wrote:
> On 02/02/2018 10:32 AM, Arnd Bergmann wrote:
>> 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>
>> ---
>> drivers/xen/fallback.c | 94 ++++++++++++++++++++++++++++----------------------
>> 1 file changed, 53 insertions(+), 41 deletions(-)
>>
>> default:
>> - WARN_ON(rc != -ENOSYS);
>> - break;
>> + return -ENOSYS;
>> }
>>
>> + memcpy(&op.u, arg, len);
>> + rc = _hypercall1(int, event_channel_op_compat, &op);
>> + memcpy(arg, &op.u, len);
>
>
> We don't copy back for all commands, only those that are COPY_BACK.
Not sure what you mean. Is it harmful to copy back the data for the others
in any way? Otherwise I wouldn't micro-optimize this.
Arnd
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH] xen: hypercall: fix out-of-bounds memcpy
2018-02-03 15:12 ` Arnd Bergmann
@ 2018-02-03 17:08 ` Boris Ostrovsky
2018-02-04 15:35 ` Arnd Bergmann
2018-02-04 15:35 ` Arnd Bergmann
2018-02-03 17:08 ` Boris Ostrovsky
1 sibling, 2 replies; 26+ messages in thread
From: Boris Ostrovsky @ 2018-02-03 17:08 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Juergen Gross, Nicolas Pitre, Andi Kleen, Dan Carpenter,
Jan Beulich, xen-devel, Linux Kernel Mailing List
On 02/03/2018 10:12 AM, Arnd Bergmann wrote:
> On Sat, Feb 3, 2018 at 12:33 AM, Boris Ostrovsky
> <boris.ostrovsky@oracle.com> wrote:
>> On 02/02/2018 10:32 AM, Arnd Bergmann wrote:
>>> 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>
>>> ---
>>> drivers/xen/fallback.c | 94 ++++++++++++++++++++++++++++----------------------
>>> 1 file changed, 53 insertions(+), 41 deletions(-)
>>>
>
>>> default:
>>> - WARN_ON(rc != -ENOSYS);
>>> - break;
>>> + return -ENOSYS;
>>> }
>>>
>>> + memcpy(&op.u, arg, len);
>>> + rc = _hypercall1(int, event_channel_op_compat, &op);
>>> + memcpy(arg, &op.u, len);
>>
>>
>> We don't copy back for all commands, only those that are COPY_BACK.
>
> Not sure what you mean. Is it harmful to copy back the data for the others
> in any way? Otherwise I wouldn't micro-optimize this.
I should have checked the original commit for fallback.c --- the code
that it replaced was doing copybacks for all hypercalls and selective
copybacks is an optimization introduced in that commit.
-boris
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH] xen: hypercall: fix out-of-bounds memcpy
2018-02-03 17:08 ` Boris Ostrovsky
@ 2018-02-04 15:35 ` Arnd Bergmann
2018-02-04 18:55 ` Boris Ostrovsky
2018-02-04 18:55 ` Boris Ostrovsky
2018-02-04 15:35 ` Arnd Bergmann
1 sibling, 2 replies; 26+ messages in thread
From: Arnd Bergmann @ 2018-02-04 15:35 UTC (permalink / raw)
To: Boris Ostrovsky
Cc: Juergen Gross, Nicolas Pitre, Andi Kleen, Dan Carpenter,
Jan Beulich, xen-devel, Linux Kernel Mailing List
On Sat, Feb 3, 2018 at 6:08 PM, Boris Ostrovsky
<boris.ostrovsky@oracle.com> wrote:
>
>
> On 02/03/2018 10:12 AM, Arnd Bergmann wrote:
>>
>> On Sat, Feb 3, 2018 at 12:33 AM, Boris Ostrovsky
>> <boris.ostrovsky@oracle.com> wrote:
>>>
>>> On 02/02/2018 10:32 AM, Arnd Bergmann wrote:
>>>>
>>>> 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>
>>>> ---
>>>> drivers/xen/fallback.c | 94
>>>> ++++++++++++++++++++++++++++----------------------
>>>> 1 file changed, 53 insertions(+), 41 deletions(-)
>>>>
>>
>>>> default:
>>>> - WARN_ON(rc != -ENOSYS);
>>>> - break;
>>>> + return -ENOSYS;
>>>> }
>>>>
>>>> + memcpy(&op.u, arg, len);
>>>> + rc = _hypercall1(int, event_channel_op_compat, &op);
>>>> + memcpy(arg, &op.u, len);
>>>
>>>
>>>
>>> We don't copy back for all commands, only those that are COPY_BACK.
>>
>>
>> Not sure what you mean. Is it harmful to copy back the data for the others
>> in any way? Otherwise I wouldn't micro-optimize this.
>
>
>
> I should have checked the original commit for fallback.c --- the code that
> it replaced was doing copybacks for all hypercalls and selective copybacks
> is an optimization introduced in that commit.
It was not an optimization but a correctness fix to avoid overflowing
the caller stack on the copy-back operation. What I tried to explain
in my commit message is that the same fix is also needed on
the copy-out before it. It's only a read access beyond the end
of a local variable, but not both the static checks and kasan-stack
get alarmed about it.
Arnd
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH] xen: hypercall: fix out-of-bounds memcpy
2018-02-04 15:35 ` Arnd Bergmann
@ 2018-02-04 18:55 ` Boris Ostrovsky
2018-02-04 18:55 ` Boris Ostrovsky
1 sibling, 0 replies; 26+ messages in thread
From: Boris Ostrovsky @ 2018-02-04 18:55 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Juergen Gross, Andi Kleen, Nicolas Pitre,
Linux Kernel Mailing List, Jan Beulich, xen-devel, Dan Carpenter
On 02/04/2018 10:35 AM, Arnd Bergmann wrote:
> On Sat, Feb 3, 2018 at 6:08 PM, Boris Ostrovsky
> <boris.ostrovsky@oracle.com> wrote:
>>
>> On 02/03/2018 10:12 AM, Arnd Bergmann wrote:
>>> On Sat, Feb 3, 2018 at 12:33 AM, Boris Ostrovsky
>>> <boris.ostrovsky@oracle.com> wrote:
>>>> On 02/02/2018 10:32 AM, Arnd Bergmann wrote:
>>>>> 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>
>>>>> ---
>>>>> drivers/xen/fallback.c | 94
>>>>> ++++++++++++++++++++++++++++----------------------
>>>>> 1 file changed, 53 insertions(+), 41 deletions(-)
>>>>>
>>>>> default:
>>>>> - WARN_ON(rc != -ENOSYS);
>>>>> - break;
>>>>> + return -ENOSYS;
>>>>> }
>>>>>
>>>>> + memcpy(&op.u, arg, len);
>>>>> + rc = _hypercall1(int, event_channel_op_compat, &op);
>>>>> + memcpy(arg, &op.u, len);
>>>>
>>>>
>>>> We don't copy back for all commands, only those that are COPY_BACK.
>>>
>>> Not sure what you mean. Is it harmful to copy back the data for the others
>>> in any way? Otherwise I wouldn't micro-optimize this.
>>
>>
>> I should have checked the original commit for fallback.c --- the code that
>> it replaced was doing copybacks for all hypercalls and selective copybacks
>> is an optimization introduced in that commit.
> It was not an optimization but a correctness fix to avoid overflowing
> the caller stack on the copy-back operation. What I tried to explain
> in my commit message is that the same fix is also needed on
> the copy-out before it. It's only a read access beyond the end
> of a local variable, but not both the static checks and kasan-stack
> get alarmed about it.
>
Yes, I understand that. I was referring to:
Move the fallback code into out-of-line functions, and handle all of
the operations known by this old a hypervisor individually: *Some don't
require copying back anything at all*, and for the rest use the
individual argument structures' sizes rather than the container's
in the original commit. I.e. prior to that commit we *were* copying back
for all commands (although possibly with potentially incorrect size). So
not copying back for some commands was an optimization.
In any case,
Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH] xen: hypercall: fix out-of-bounds memcpy
2018-02-04 15:35 ` Arnd Bergmann
2018-02-04 18:55 ` Boris Ostrovsky
@ 2018-02-04 18:55 ` Boris Ostrovsky
1 sibling, 0 replies; 26+ messages in thread
From: Boris Ostrovsky @ 2018-02-04 18:55 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Juergen Gross, Nicolas Pitre, Andi Kleen, Dan Carpenter,
Jan Beulich, xen-devel, Linux Kernel Mailing List
On 02/04/2018 10:35 AM, Arnd Bergmann wrote:
> On Sat, Feb 3, 2018 at 6:08 PM, Boris Ostrovsky
> <boris.ostrovsky@oracle.com> wrote:
>>
>> On 02/03/2018 10:12 AM, Arnd Bergmann wrote:
>>> On Sat, Feb 3, 2018 at 12:33 AM, Boris Ostrovsky
>>> <boris.ostrovsky@oracle.com> wrote:
>>>> On 02/02/2018 10:32 AM, Arnd Bergmann wrote:
>>>>> 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>
>>>>> ---
>>>>> drivers/xen/fallback.c | 94
>>>>> ++++++++++++++++++++++++++++----------------------
>>>>> 1 file changed, 53 insertions(+), 41 deletions(-)
>>>>>
>>>>> default:
>>>>> - WARN_ON(rc != -ENOSYS);
>>>>> - break;
>>>>> + return -ENOSYS;
>>>>> }
>>>>>
>>>>> + memcpy(&op.u, arg, len);
>>>>> + rc = _hypercall1(int, event_channel_op_compat, &op);
>>>>> + memcpy(arg, &op.u, len);
>>>>
>>>>
>>>> We don't copy back for all commands, only those that are COPY_BACK.
>>>
>>> Not sure what you mean. Is it harmful to copy back the data for the others
>>> in any way? Otherwise I wouldn't micro-optimize this.
>>
>>
>> I should have checked the original commit for fallback.c --- the code that
>> it replaced was doing copybacks for all hypercalls and selective copybacks
>> is an optimization introduced in that commit.
> It was not an optimization but a correctness fix to avoid overflowing
> the caller stack on the copy-back operation. What I tried to explain
> in my commit message is that the same fix is also needed on
> the copy-out before it. It's only a read access beyond the end
> of a local variable, but not both the static checks and kasan-stack
> get alarmed about it.
>
Yes, I understand that. I was referring to:
Move the fallback code into out-of-line functions, and handle all of
the operations known by this old a hypervisor individually: *Some don't
require copying back anything at all*, and for the rest use the
individual argument structures' sizes rather than the container's
in the original commit. I.e. prior to that commit we *were* copying back
for all commands (although possibly with potentially incorrect size). So
not copying back for some commands was an optimization.
In any case,
Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] xen: hypercall: fix out-of-bounds memcpy
2018-02-03 17:08 ` Boris Ostrovsky
2018-02-04 15:35 ` Arnd Bergmann
@ 2018-02-04 15:35 ` Arnd Bergmann
1 sibling, 0 replies; 26+ messages in thread
From: Arnd Bergmann @ 2018-02-04 15:35 UTC (permalink / raw)
To: Boris Ostrovsky
Cc: Juergen Gross, Andi Kleen, Nicolas Pitre,
Linux Kernel Mailing List, Jan Beulich, xen-devel, Dan Carpenter
On Sat, Feb 3, 2018 at 6:08 PM, Boris Ostrovsky
<boris.ostrovsky@oracle.com> wrote:
>
>
> On 02/03/2018 10:12 AM, Arnd Bergmann wrote:
>>
>> On Sat, Feb 3, 2018 at 12:33 AM, Boris Ostrovsky
>> <boris.ostrovsky@oracle.com> wrote:
>>>
>>> On 02/02/2018 10:32 AM, Arnd Bergmann wrote:
>>>>
>>>> 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>
>>>> ---
>>>> drivers/xen/fallback.c | 94
>>>> ++++++++++++++++++++++++++++----------------------
>>>> 1 file changed, 53 insertions(+), 41 deletions(-)
>>>>
>>
>>>> default:
>>>> - WARN_ON(rc != -ENOSYS);
>>>> - break;
>>>> + return -ENOSYS;
>>>> }
>>>>
>>>> + memcpy(&op.u, arg, len);
>>>> + rc = _hypercall1(int, event_channel_op_compat, &op);
>>>> + memcpy(arg, &op.u, len);
>>>
>>>
>>>
>>> We don't copy back for all commands, only those that are COPY_BACK.
>>
>>
>> Not sure what you mean. Is it harmful to copy back the data for the others
>> in any way? Otherwise I wouldn't micro-optimize this.
>
>
>
> I should have checked the original commit for fallback.c --- the code that
> it replaced was doing copybacks for all hypercalls and selective copybacks
> is an optimization introduced in that commit.
It was not an optimization but a correctness fix to avoid overflowing
the caller stack on the copy-back operation. What I tried to explain
in my commit message is that the same fix is also needed on
the copy-out before it. It's only a read access beyond the end
of a local variable, but not both the static checks and kasan-stack
get alarmed about it.
Arnd
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] xen: hypercall: fix out-of-bounds memcpy
2018-02-03 15:12 ` Arnd Bergmann
2018-02-03 17:08 ` Boris Ostrovsky
@ 2018-02-03 17:08 ` Boris Ostrovsky
1 sibling, 0 replies; 26+ messages in thread
From: Boris Ostrovsky @ 2018-02-03 17:08 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Juergen Gross, Andi Kleen, Nicolas Pitre,
Linux Kernel Mailing List, Jan Beulich, xen-devel, Dan Carpenter
On 02/03/2018 10:12 AM, Arnd Bergmann wrote:
> On Sat, Feb 3, 2018 at 12:33 AM, Boris Ostrovsky
> <boris.ostrovsky@oracle.com> wrote:
>> On 02/02/2018 10:32 AM, Arnd Bergmann wrote:
>>> 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>
>>> ---
>>> drivers/xen/fallback.c | 94 ++++++++++++++++++++++++++++----------------------
>>> 1 file changed, 53 insertions(+), 41 deletions(-)
>>>
>
>>> default:
>>> - WARN_ON(rc != -ENOSYS);
>>> - break;
>>> + return -ENOSYS;
>>> }
>>>
>>> + memcpy(&op.u, arg, len);
>>> + rc = _hypercall1(int, event_channel_op_compat, &op);
>>> + memcpy(arg, &op.u, len);
>>
>>
>> We don't copy back for all commands, only those that are COPY_BACK.
>
> Not sure what you mean. Is it harmful to copy back the data for the others
> in any way? Otherwise I wouldn't micro-optimize this.
I should have checked the original commit for fallback.c --- the code
that it replaced was doing copybacks for all hypercalls and selective
copybacks is an optimization introduced in that commit.
-boris
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] xen: hypercall: fix out-of-bounds memcpy
2018-02-02 23:33 ` Boris Ostrovsky
2018-02-03 15:12 ` Arnd Bergmann
2018-02-03 15:12 ` Arnd Bergmann
@ 2018-02-05 12:11 ` David Laight
2018-02-05 12:11 ` David Laight
3 siblings, 0 replies; 26+ messages in thread
From: David Laight @ 2018-02-05 12:11 UTC (permalink / raw)
To: 'Boris Ostrovsky', Arnd Bergmann, Juergen Gross
Cc: Andi Kleen, Nicolas Pitre, linux-kernel, Jan Beulich, xen-devel,
Dan Carpenter
From: Boris Ostrovsky
> Sent: 02 February 2018 23:34
...
> > switch (cmd) {
> > + case EVTCHNOP_bind_interdomain:
> > + len = sizeof(struct evtchn_bind_interdomain);
> > + break;
> > + case EVTCHNOP_bind_virq:
> > + len = sizeof(struct evtchn_bind_virq);
> > + break;
> > + case EVTCHNOP_bind_pirq:
> > + len = sizeof(struct evtchn_bind_pirq);
> > + break;
> > case EVTCHNOP_close:
> > + len = sizeof(struct evtchn_close);
> > + break;
> > case EVTCHNOP_send:
> > + len = sizeof(struct evtchn_send);
> > + break;
> > + case EVTCHNOP_alloc_unbound:
> > + len = sizeof(struct evtchn_alloc_unbound);
> > + break;
> > + case EVTCHNOP_bind_ipi:
> > + len = sizeof(struct evtchn_bind_ipi);
> > + break;
> > + case EVTCHNOP_status:
> > + len = sizeof(struct evtchn_status);
> > + break;
> > case EVTCHNOP_bind_vcpu:
> > + len = sizeof(struct evtchn_bind_vcpu);
> > + break;
> > case EVTCHNOP_unmask:
> > - /* no output */
> > + len = sizeof(struct evtchn_unmask);
> > break;
Are the EVTCHNOP_xxx values dense?
In which case an array is almost certainly better than the switch statement.
David
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 26+ messages in thread* RE: [PATCH] xen: hypercall: fix out-of-bounds memcpy
2018-02-02 23:33 ` Boris Ostrovsky
` (2 preceding siblings ...)
2018-02-05 12:11 ` David Laight
@ 2018-02-05 12:11 ` David Laight
2018-02-05 12:37 ` Arnd Bergmann
2018-02-05 12:37 ` Arnd Bergmann
3 siblings, 2 replies; 26+ messages in thread
From: David Laight @ 2018-02-05 12:11 UTC (permalink / raw)
To: 'Boris Ostrovsky', Arnd Bergmann, Juergen Gross
Cc: Nicolas Pitre, Andi Kleen, Dan Carpenter, Jan Beulich, xen-devel,
linux-kernel
From: Boris Ostrovsky
> Sent: 02 February 2018 23:34
...
> > switch (cmd) {
> > + case EVTCHNOP_bind_interdomain:
> > + len = sizeof(struct evtchn_bind_interdomain);
> > + break;
> > + case EVTCHNOP_bind_virq:
> > + len = sizeof(struct evtchn_bind_virq);
> > + break;
> > + case EVTCHNOP_bind_pirq:
> > + len = sizeof(struct evtchn_bind_pirq);
> > + break;
> > case EVTCHNOP_close:
> > + len = sizeof(struct evtchn_close);
> > + break;
> > case EVTCHNOP_send:
> > + len = sizeof(struct evtchn_send);
> > + break;
> > + case EVTCHNOP_alloc_unbound:
> > + len = sizeof(struct evtchn_alloc_unbound);
> > + break;
> > + case EVTCHNOP_bind_ipi:
> > + len = sizeof(struct evtchn_bind_ipi);
> > + break;
> > + case EVTCHNOP_status:
> > + len = sizeof(struct evtchn_status);
> > + break;
> > case EVTCHNOP_bind_vcpu:
> > + len = sizeof(struct evtchn_bind_vcpu);
> > + break;
> > case EVTCHNOP_unmask:
> > - /* no output */
> > + len = sizeof(struct evtchn_unmask);
> > break;
Are the EVTCHNOP_xxx values dense?
In which case an array is almost certainly better than the switch statement.
David
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH] xen: hypercall: fix out-of-bounds memcpy
2018-02-05 12:11 ` David Laight
@ 2018-02-05 12:37 ` Arnd Bergmann
2018-02-05 12:37 ` Arnd Bergmann
1 sibling, 0 replies; 26+ messages in thread
From: Arnd Bergmann @ 2018-02-05 12:37 UTC (permalink / raw)
To: David Laight
Cc: Juergen Gross, Andi Kleen, Nicolas Pitre, linux-kernel,
Jan Beulich, xen-devel, Boris Ostrovsky, Dan Carpenter
On Mon, Feb 5, 2018 at 1:11 PM, David Laight <David.Laight@aculab.com> wrote:
> From: Boris Ostrovsky
>> Sent: 02 February 2018 23:34
> ...
>> > switch (cmd) {
>> > + case EVTCHNOP_bind_interdomain:
>> > + len = sizeof(struct evtchn_bind_interdomain);
>> > + break;
>> > + case EVTCHNOP_bind_virq:
>> > + len = sizeof(struct evtchn_bind_virq);
>> > + break;
>> > + case EVTCHNOP_bind_pirq:
>> > + len = sizeof(struct evtchn_bind_pirq);
>> > + break;
>> > case EVTCHNOP_close:
>> > + len = sizeof(struct evtchn_close);
>> > + break;
>> > case EVTCHNOP_send:
>> > + len = sizeof(struct evtchn_send);
>> > + break;
>> > + case EVTCHNOP_alloc_unbound:
>> > + len = sizeof(struct evtchn_alloc_unbound);
>> > + break;
>> > + case EVTCHNOP_bind_ipi:
>> > + len = sizeof(struct evtchn_bind_ipi);
>> > + break;
>> > + case EVTCHNOP_status:
>> > + len = sizeof(struct evtchn_status);
>> > + break;
>> > case EVTCHNOP_bind_vcpu:
>> > + len = sizeof(struct evtchn_bind_vcpu);
>> > + break;
>> > case EVTCHNOP_unmask:
>> > - /* no output */
>> > + len = sizeof(struct evtchn_unmask);
>> > break;
>
> Are the EVTCHNOP_xxx values dense?
> In which case an array is almost certainly better than the switch statement.
They are, yes. PHYSDEVOP_xxx are also consecutive by start at '4'.
Dan made the same comment earlier, and I replied that my I had
considered it but went for the more failsafe route. I also verified my
assumption now that gcc in fact is smart enough to turn this
into a table by itself:
xen_physdev_op_compat:
pushq %r13 #
leal -4(%rdi), %eax #, _2
pushq %r12 #
pushq %rbp #
pushq %rbx #
subq $24, %rsp #,
# /git/arm-soc/drivers/xen/fallback.c:59: struct physdev_op op =
{ .cmd = cmd, };
movq $0, 4(%rsp) #, MEM[(struct physdev_op *)&op + 4B]
movq $0, 12(%rsp) #, MEM[(struct physdev_op *)&op + 4B]
movl $0, 20(%rsp) #, MEM[(struct physdev_op *)&op + 4B]
movl %edi, (%rsp) # cmd, op.cmd
cmpl $6, %eax #, _2
ja .L8 #,
movl %eax, %edi # _2, _2
# /git/arm-soc/drivers/xen/fallback.c:87: memcpy(&op.u, arg, len);
leaq 8(%rsp), %r12 #, tmp98
movq %rsi, %rbx # arg, arg
movq CSWTCH.17(,%rdi,8), %r13 # CSWTCH.17, _5
movq %r12, %rdi # tmp98,
movq %r13, %rdx # _5,
call __memcpy #
# /git/arm-soc/drivers/xen/fallback.c:88: rc = _hypercall1(int,
physdev_op_compat, &op);
movq %rsp, %rdi #, __arg1
#APP
# 88 "/git/arm-soc/drivers/xen/fallback.c" 1
call hypercall_page+608 #
Arnd
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH] xen: hypercall: fix out-of-bounds memcpy
2018-02-05 12:11 ` David Laight
2018-02-05 12:37 ` Arnd Bergmann
@ 2018-02-05 12:37 ` Arnd Bergmann
2018-02-05 13:58 ` David Laight
2018-02-05 13:58 ` David Laight
1 sibling, 2 replies; 26+ messages in thread
From: Arnd Bergmann @ 2018-02-05 12:37 UTC (permalink / raw)
To: David Laight
Cc: Boris Ostrovsky, Juergen Gross, Nicolas Pitre, Andi Kleen,
Dan Carpenter, Jan Beulich, xen-devel, linux-kernel
On Mon, Feb 5, 2018 at 1:11 PM, David Laight <David.Laight@aculab.com> wrote:
> From: Boris Ostrovsky
>> Sent: 02 February 2018 23:34
> ...
>> > switch (cmd) {
>> > + case EVTCHNOP_bind_interdomain:
>> > + len = sizeof(struct evtchn_bind_interdomain);
>> > + break;
>> > + case EVTCHNOP_bind_virq:
>> > + len = sizeof(struct evtchn_bind_virq);
>> > + break;
>> > + case EVTCHNOP_bind_pirq:
>> > + len = sizeof(struct evtchn_bind_pirq);
>> > + break;
>> > case EVTCHNOP_close:
>> > + len = sizeof(struct evtchn_close);
>> > + break;
>> > case EVTCHNOP_send:
>> > + len = sizeof(struct evtchn_send);
>> > + break;
>> > + case EVTCHNOP_alloc_unbound:
>> > + len = sizeof(struct evtchn_alloc_unbound);
>> > + break;
>> > + case EVTCHNOP_bind_ipi:
>> > + len = sizeof(struct evtchn_bind_ipi);
>> > + break;
>> > + case EVTCHNOP_status:
>> > + len = sizeof(struct evtchn_status);
>> > + break;
>> > case EVTCHNOP_bind_vcpu:
>> > + len = sizeof(struct evtchn_bind_vcpu);
>> > + break;
>> > case EVTCHNOP_unmask:
>> > - /* no output */
>> > + len = sizeof(struct evtchn_unmask);
>> > break;
>
> Are the EVTCHNOP_xxx values dense?
> In which case an array is almost certainly better than the switch statement.
They are, yes. PHYSDEVOP_xxx are also consecutive by start at '4'.
Dan made the same comment earlier, and I replied that my I had
considered it but went for the more failsafe route. I also verified my
assumption now that gcc in fact is smart enough to turn this
into a table by itself:
xen_physdev_op_compat:
pushq %r13 #
leal -4(%rdi), %eax #, _2
pushq %r12 #
pushq %rbp #
pushq %rbx #
subq $24, %rsp #,
# /git/arm-soc/drivers/xen/fallback.c:59: struct physdev_op op =
{ .cmd = cmd, };
movq $0, 4(%rsp) #, MEM[(struct physdev_op *)&op + 4B]
movq $0, 12(%rsp) #, MEM[(struct physdev_op *)&op + 4B]
movl $0, 20(%rsp) #, MEM[(struct physdev_op *)&op + 4B]
movl %edi, (%rsp) # cmd, op.cmd
cmpl $6, %eax #, _2
ja .L8 #,
movl %eax, %edi # _2, _2
# /git/arm-soc/drivers/xen/fallback.c:87: memcpy(&op.u, arg, len);
leaq 8(%rsp), %r12 #, tmp98
movq %rsi, %rbx # arg, arg
movq CSWTCH.17(,%rdi,8), %r13 # CSWTCH.17, _5
movq %r12, %rdi # tmp98,
movq %r13, %rdx # _5,
call __memcpy #
# /git/arm-soc/drivers/xen/fallback.c:88: rc = _hypercall1(int,
physdev_op_compat, &op);
movq %rsp, %rdi #, __arg1
#APP
# 88 "/git/arm-soc/drivers/xen/fallback.c" 1
call hypercall_page+608 #
Arnd
^ permalink raw reply [flat|nested] 26+ messages in thread* RE: [PATCH] xen: hypercall: fix out-of-bounds memcpy
2018-02-05 12:37 ` Arnd Bergmann
@ 2018-02-05 13:58 ` David Laight
2018-02-05 14:18 ` Arnd Bergmann
2018-02-05 14:18 ` Arnd Bergmann
2018-02-05 13:58 ` David Laight
1 sibling, 2 replies; 26+ messages in thread
From: David Laight @ 2018-02-05 13:58 UTC (permalink / raw)
To: 'Arnd Bergmann'
Cc: Boris Ostrovsky, Juergen Gross, Nicolas Pitre, Andi Kleen,
Dan Carpenter, Jan Beulich, xen-devel, linux-kernel
From: Arnd Bergmann
> Sent: 05 February 2018 12:37
....
> > Are the EVTCHNOP_xxx values dense?
> > In which case an array is almost certainly better than the switch statement.
>
> They are, yes. PHYSDEVOP_xxx are also consecutive by start at '4'.
> Dan made the same comment earlier, and I replied that my I had
> considered it but went for the more failsafe route. I also verified my
> assumption now that gcc in fact is smart enough to turn this
> into a table by itself:
I've never spotted that optimisation, must be fairly new.
David
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] xen: hypercall: fix out-of-bounds memcpy
2018-02-05 13:58 ` David Laight
@ 2018-02-05 14:18 ` Arnd Bergmann
2018-02-05 14:18 ` Arnd Bergmann
1 sibling, 0 replies; 26+ messages in thread
From: Arnd Bergmann @ 2018-02-05 14:18 UTC (permalink / raw)
To: David Laight
Cc: Juergen Gross, Andi Kleen, Nicolas Pitre, linux-kernel,
Jan Beulich, xen-devel, Boris Ostrovsky, Dan Carpenter
On Mon, Feb 5, 2018 at 2:58 PM, David Laight <David.Laight@aculab.com> wrote:
> From: Arnd Bergmann
>> Sent: 05 February 2018 12:37
> ....
>> > Are the EVTCHNOP_xxx values dense?
>> > In which case an array is almost certainly better than the switch statement.
>>
>> They are, yes. PHYSDEVOP_xxx are also consecutive by start at '4'.
>> Dan made the same comment earlier, and I replied that my I had
>> considered it but went for the more failsafe route. I also verified my
>> assumption now that gcc in fact is smart enough to turn this
>> into a table by itself:
>
> I've never spotted that optimisation, must be fairly new.
Indeed, I checked again and found that most compilers have a
less efficient jump table based version, the output I posted was
from gcc-8, this is what I get with gcc-4.8 through gcc-7:
xen_event_channel_op_compat:
pushq %r13 #
pushq %r12 #
pushq %rbp #
pushq %rbx #
movl $-38, %ebx #, <retval>
subq $32, %rsp #,
# /git/arm-soc/drivers/xen/fallback.c:14: switch (cmd) {
cmpl $9, %edi #, cmd
# /git/arm-soc/drivers/xen/fallback.c:10: struct evtchn_op op =
{ .cmd = cmd, };
movq $0, 8(%rsp) #, MEM[(struct evtchn_op *)&op + 4B]
movq $0, 16(%rsp) #, MEM[(struct evtchn_op *)&op + 4B]
movq $0, 24(%rsp) #, MEM[(struct evtchn_op *)&op + 4B]
movl %edi, 4(%rsp) # cmd, op.cmd
# /git/arm-soc/drivers/xen/fallback.c:14: switch (cmd) {
ja .L1 #,
movl %edi, %eax # cmd, cmd
jmp *.L4(,%rax,8) #
.section .rodata
.align 8
.align 4
.L4:
.quad .L9
.quad .L9
.quad .L9
.quad .L5
.quad .L5
.quad .L6
.quad .L7
.quad .L7
.quad .L7
.quad .L5
.text
.L7:
# /git/arm-soc/drivers/xen/fallback.c:31: len =
sizeof(struct evtchn_alloc_unbound);
movl $8, %r12d #, len
.L3:
# /git/arm-soc/drivers/xen/fallback.c:49: memcpy(&op.u, arg, len);
leaq 8(%rsp), %r13 #, tmp98
movq %r12, %rdx # len,
movq %rsi, %rbp # arg, arg
movq %r13, %rdi # tmp98,
call __memcpy #
# /git/arm-soc/drivers/xen/fallback.c:50: rc = _hypercall1(int,
event_channel_op_compat, &op);
leaq 4(%rsp), %rdi #, tmp104
#APP
# 50 "/git/arm-soc/drivers/xen/fallback.c" 1
call hypercall_page+512 #
# 0 "" 2
# /git/arm-soc/drivers/xen/fallback.c:51: memcpy(arg, &op.u, len);
#NO_APP
movq %r12, %rdx # len,
movq %r13, %rsi # tmp98,
movq %rbp, %rdi # arg,
# /git/arm-soc/drivers/xen/fallback.c:50: rc = _hypercall1(int,
event_channel_op_compat, &op);
movl %eax, %ebx # __res.7_3, <retval>
# /git/arm-soc/drivers/xen/fallback.c:51: memcpy(arg, &op.u, len);
call __memcpy #
.L1:
# /git/arm-soc/drivers/xen/fallback.c:54: }
addq $32, %rsp #,
movl %ebx, %eax # <retval>,
popq %rbx #
popq %rbp #
popq %r12 #
popq %r13 #
ret
.L5:
# /git/arm-soc/drivers/xen/fallback.c:25: len =
sizeof(struct evtchn_close);
movl $4, %r12d #, len
jmp .L3 #
.L9:
# /git/arm-soc/drivers/xen/fallback.c:16: len =
sizeof(struct evtchn_bind_interdomain);
movl $12, %r12d #, len
jmp .L3 #
.L6:
# /git/arm-soc/drivers/xen/fallback.c:37: len =
sizeof(struct evtchn_status);
movl $24, %r12d #, len
# /git/arm-soc/drivers/xen/fallback.c:38: break;
jmp .L3 #
.size xen_event_channel_op_compat, .-xen_event_channel_op_compat
.p2align 4,,15
which isn't all that bad, but gets slightly worse when you compile with
-mindirect-branch=thunk-extern, the total size now grows from 474
bytes with gcc-8 to 525 bytes with gcc-7+retpoline.
Arnd
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH] xen: hypercall: fix out-of-bounds memcpy
2018-02-05 13:58 ` David Laight
2018-02-05 14:18 ` Arnd Bergmann
@ 2018-02-05 14:18 ` Arnd Bergmann
1 sibling, 0 replies; 26+ messages in thread
From: Arnd Bergmann @ 2018-02-05 14:18 UTC (permalink / raw)
To: David Laight
Cc: Boris Ostrovsky, Juergen Gross, Nicolas Pitre, Andi Kleen,
Dan Carpenter, Jan Beulich, xen-devel, linux-kernel
On Mon, Feb 5, 2018 at 2:58 PM, David Laight <David.Laight@aculab.com> wrote:
> From: Arnd Bergmann
>> Sent: 05 February 2018 12:37
> ....
>> > Are the EVTCHNOP_xxx values dense?
>> > In which case an array is almost certainly better than the switch statement.
>>
>> They are, yes. PHYSDEVOP_xxx are also consecutive by start at '4'.
>> Dan made the same comment earlier, and I replied that my I had
>> considered it but went for the more failsafe route. I also verified my
>> assumption now that gcc in fact is smart enough to turn this
>> into a table by itself:
>
> I've never spotted that optimisation, must be fairly new.
Indeed, I checked again and found that most compilers have a
less efficient jump table based version, the output I posted was
from gcc-8, this is what I get with gcc-4.8 through gcc-7:
xen_event_channel_op_compat:
pushq %r13 #
pushq %r12 #
pushq %rbp #
pushq %rbx #
movl $-38, %ebx #, <retval>
subq $32, %rsp #,
# /git/arm-soc/drivers/xen/fallback.c:14: switch (cmd) {
cmpl $9, %edi #, cmd
# /git/arm-soc/drivers/xen/fallback.c:10: struct evtchn_op op =
{ .cmd = cmd, };
movq $0, 8(%rsp) #, MEM[(struct evtchn_op *)&op + 4B]
movq $0, 16(%rsp) #, MEM[(struct evtchn_op *)&op + 4B]
movq $0, 24(%rsp) #, MEM[(struct evtchn_op *)&op + 4B]
movl %edi, 4(%rsp) # cmd, op.cmd
# /git/arm-soc/drivers/xen/fallback.c:14: switch (cmd) {
ja .L1 #,
movl %edi, %eax # cmd, cmd
jmp *.L4(,%rax,8) #
.section .rodata
.align 8
.align 4
.L4:
.quad .L9
.quad .L9
.quad .L9
.quad .L5
.quad .L5
.quad .L6
.quad .L7
.quad .L7
.quad .L7
.quad .L5
.text
.L7:
# /git/arm-soc/drivers/xen/fallback.c:31: len =
sizeof(struct evtchn_alloc_unbound);
movl $8, %r12d #, len
.L3:
# /git/arm-soc/drivers/xen/fallback.c:49: memcpy(&op.u, arg, len);
leaq 8(%rsp), %r13 #, tmp98
movq %r12, %rdx # len,
movq %rsi, %rbp # arg, arg
movq %r13, %rdi # tmp98,
call __memcpy #
# /git/arm-soc/drivers/xen/fallback.c:50: rc = _hypercall1(int,
event_channel_op_compat, &op);
leaq 4(%rsp), %rdi #, tmp104
#APP
# 50 "/git/arm-soc/drivers/xen/fallback.c" 1
call hypercall_page+512 #
# 0 "" 2
# /git/arm-soc/drivers/xen/fallback.c:51: memcpy(arg, &op.u, len);
#NO_APP
movq %r12, %rdx # len,
movq %r13, %rsi # tmp98,
movq %rbp, %rdi # arg,
# /git/arm-soc/drivers/xen/fallback.c:50: rc = _hypercall1(int,
event_channel_op_compat, &op);
movl %eax, %ebx # __res.7_3, <retval>
# /git/arm-soc/drivers/xen/fallback.c:51: memcpy(arg, &op.u, len);
call __memcpy #
.L1:
# /git/arm-soc/drivers/xen/fallback.c:54: }
addq $32, %rsp #,
movl %ebx, %eax # <retval>,
popq %rbx #
popq %rbp #
popq %r12 #
popq %r13 #
ret
.L5:
# /git/arm-soc/drivers/xen/fallback.c:25: len =
sizeof(struct evtchn_close);
movl $4, %r12d #, len
jmp .L3 #
.L9:
# /git/arm-soc/drivers/xen/fallback.c:16: len =
sizeof(struct evtchn_bind_interdomain);
movl $12, %r12d #, len
jmp .L3 #
.L6:
# /git/arm-soc/drivers/xen/fallback.c:37: len =
sizeof(struct evtchn_status);
movl $24, %r12d #, len
# /git/arm-soc/drivers/xen/fallback.c:38: break;
jmp .L3 #
.size xen_event_channel_op_compat, .-xen_event_channel_op_compat
.p2align 4,,15
which isn't all that bad, but gets slightly worse when you compile with
-mindirect-branch=thunk-extern, the total size now grows from 474
bytes with gcc-8 to 525 bytes with gcc-7+retpoline.
Arnd
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] xen: hypercall: fix out-of-bounds memcpy
2018-02-05 12:37 ` Arnd Bergmann
2018-02-05 13:58 ` David Laight
@ 2018-02-05 13:58 ` David Laight
1 sibling, 0 replies; 26+ messages in thread
From: David Laight @ 2018-02-05 13:58 UTC (permalink / raw)
To: 'Arnd Bergmann'
Cc: Juergen Gross, Andi Kleen, Nicolas Pitre, linux-kernel,
Jan Beulich, xen-devel, Boris Ostrovsky, Dan Carpenter
From: Arnd Bergmann
> Sent: 05 February 2018 12:37
....
> > Are the EVTCHNOP_xxx values dense?
> > In which case an array is almost certainly better than the switch statement.
>
> They are, yes. PHYSDEVOP_xxx are also consecutive by start at '4'.
> Dan made the same comment earlier, and I replied that my I had
> considered it but went for the more failsafe route. I also verified my
> assumption now that gcc in fact is smart enough to turn this
> into a table by itself:
I've never spotted that optimisation, must be fairly new.
David
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] xen: hypercall: fix out-of-bounds memcpy
2018-02-02 15:32 Arnd Bergmann
` (2 preceding siblings ...)
2018-02-02 23:33 ` Boris Ostrovsky
@ 2018-02-02 23:33 ` Boris Ostrovsky
3 siblings, 0 replies; 26+ messages in thread
From: Boris Ostrovsky @ 2018-02-02 23:33 UTC (permalink / raw)
To: Arnd Bergmann, Juergen Gross
Cc: Andi Kleen, Nicolas Pitre, linux-kernel, Jan Beulich, xen-devel,
Dan Carpenter
On 02/02/2018 10:32 AM, Arnd Bergmann wrote:
> 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>
> ---
> drivers/xen/fallback.c | 94 ++++++++++++++++++++++++++++----------------------
> 1 file changed, 53 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/xen/fallback.c b/drivers/xen/fallback.c
> index b04fb64c5a91..eded8dd821ad 100644
> --- a/drivers/xen/fallback.c
> +++ b/drivers/xen/fallback.c
> @@ -7,75 +7,87 @@
>
> 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_bind_interdomain:
> + len = sizeof(struct evtchn_bind_interdomain);
> + break;
> + case EVTCHNOP_bind_virq:
> + len = sizeof(struct evtchn_bind_virq);
> + break;
> + case EVTCHNOP_bind_pirq:
> + len = sizeof(struct evtchn_bind_pirq);
> + break;
> case EVTCHNOP_close:
> + len = sizeof(struct evtchn_close);
> + break;
> case EVTCHNOP_send:
> + len = sizeof(struct evtchn_send);
> + break;
> + case EVTCHNOP_alloc_unbound:
> + len = sizeof(struct evtchn_alloc_unbound);
> + break;
> + case EVTCHNOP_bind_ipi:
> + len = sizeof(struct evtchn_bind_ipi);
> + break;
> + case EVTCHNOP_status:
> + len = sizeof(struct evtchn_status);
> + break;
> case EVTCHNOP_bind_vcpu:
> + len = sizeof(struct evtchn_bind_vcpu);
> + break;
> case EVTCHNOP_unmask:
> - /* no output */
> + len = sizeof(struct evtchn_unmask);
> break;
> -
> -#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;
> + return -ENOSYS;
> }
>
> + memcpy(&op.u, arg, len);
> + rc = _hypercall1(int, event_channel_op_compat, &op);
> + memcpy(arg, &op.u, len);
We don't copy back for all commands, only those that are COPY_BACK.
> +
> return rc;
> }
> EXPORT_SYMBOL_GPL(xen_event_channel_op_compat);
>
> 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:
> + len = 0;
> + break;
> + case PHYSDEVOP_irq_status_query:
> + len = sizeof(struct physdev_irq_status_query);
> + break;
> case PHYSDEVOP_set_iopl:
> + len = sizeof(struct physdev_set_iopl);
> + break;
> case PHYSDEVOP_set_iobitmap:
> + len = sizeof(struct physdev_set_iobitmap);
> + break;
> + case PHYSDEVOP_apic_read:
> case PHYSDEVOP_apic_write:
> - /* no output */
> + len = sizeof(struct physdev_apic);
> break;
> -
> -#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);
> + case PHYSDEVOP_ASSIGN_VECTOR:
> + len = sizeof(struct physdev_irq);
> break;
> + default:
> + return -ENOSYS;
> }
>
> + memcpy(&op.u, arg, len);
> + rc = _hypercall1(int, physdev_op_compat, &op);
> + memcpy(arg, &op.u, len);
And the same is true here.
-boris
> +
> return rc;
> }
> EXPORT_SYMBOL_GPL(xen_physdev_op_compat);
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 26+ messages in thread
end of thread, other threads:[~2018-02-05 14:18 UTC | newest]
Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-02-02 15:32 [PATCH] xen: hypercall: fix out-of-bounds memcpy Arnd Bergmann
-- strict thread matches above, loose matches on Subject: below --
2018-02-02 15:32 Arnd Bergmann
2018-02-02 15:53 ` Dan Carpenter
2018-02-02 15:53 ` Dan Carpenter
2018-02-02 16:11 ` Arnd Bergmann
2018-02-02 16:34 ` Dan Carpenter
2018-02-02 16:34 ` Dan Carpenter
2018-02-02 16:11 ` Arnd Bergmann
2018-02-02 23:33 ` Boris Ostrovsky
2018-02-03 15:12 ` Arnd Bergmann
2018-02-03 15:12 ` Arnd Bergmann
2018-02-03 17:08 ` Boris Ostrovsky
2018-02-04 15:35 ` Arnd Bergmann
2018-02-04 18:55 ` Boris Ostrovsky
2018-02-04 18:55 ` Boris Ostrovsky
2018-02-04 15:35 ` Arnd Bergmann
2018-02-03 17:08 ` Boris Ostrovsky
2018-02-05 12:11 ` David Laight
2018-02-05 12:11 ` David Laight
2018-02-05 12:37 ` Arnd Bergmann
2018-02-05 12:37 ` Arnd Bergmann
2018-02-05 13:58 ` David Laight
2018-02-05 14:18 ` Arnd Bergmann
2018-02-05 14:18 ` Arnd Bergmann
2018-02-05 13:58 ` David Laight
2018-02-02 23:33 ` Boris Ostrovsky
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.