linux-kernel - Re: [PATCH] xen: hypercall: fix out-of-bounds memcpy

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAK8P3a3pVqfuJ57eDndTTajFQchVUoas3=YcDW64xLLgvjbz4Q@mail.gmail.com>
Date:   Mon, 5 Feb 2018 15:18:00 +0100
From:   Arnd Bergmann <arnd@...db.de>
To:     David Laight <David.Laight@...lab.com>
Cc:     Boris Ostrovsky <boris.ostrovsky@...cle.com>,
        Juergen Gross <jgross@...e.com>,
        Nicolas Pitre <nico@...aro.org>,
        Andi Kleen <ak@...ux.intel.com>,
        Dan Carpenter <dan.carpenter@...cle.com>,
        Jan Beulich <jbeulich@...e.com>,
        "xen-devel@...ts.xenproject.org" <xen-devel@...ts.xenproject.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] xen: hypercall: fix out-of-bounds memcpy

On Mon, Feb 5, 2018 at 2:58 PM, David Laight <David.Laight@...lab.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