| Message ID | 20180202153240.1190361-1-arnd@arndb.de |
|---|---|
| State | New |
| Headers | show |
| Series | xen: hypercall: fix out-of-bounds memcpy | expand |
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
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
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
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);
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
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
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
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>
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
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
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
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
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);
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(-) -- 2.9.0