Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
In-Reply-To: <20180202155309.2xg2gjcp7wb7bbpe@mwanda>
References: <20180202153240.1190361-1-arnd@arndb.de> <20180202155309.2xg2gjcp7wb7bbpe@mwanda>
From:   Arnd Bergmann <arnd@arndb.de>
Date:   Fri, 2 Feb 2018 17:11:02 +0100
Message-ID: <CAK8P3a0Z2PMP-=e2Ga8kw196vWeAz_HC02ABQmDB9W_0Cmmr-Q@mail.gmail.com>
Subject: Re: [PATCH] xen: hypercall: fix out-of-bounds memcpy
To:     Dan Carpenter <dan.carpenter@oracle.com>
Cc:     Boris Ostrovsky <boris.ostrovsky@oracle.com>,
        Juergen Gross <jgross@suse.com>,
        Nicolas Pitre <nico@linaro.org>,
        Andi Kleen <ak@linux.intel.com>,
        Jan Beulich <jbeulich@suse.com>,
        xen-devel <xen-devel@lists.xenproject.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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