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;
Date:   Tue, 15 Jan 2019 09:51:11 -0800
From:   Tom Roeder <tmroeder@google.com>
To:     Sean Christopherson <sean.j.christopherson@intel.com>
Cc:     Paolo Bonzini <pbonzini@redhat.com>,
        Radim =?utf-8?B?S3LEjW3DocWZ?= <rkrcmar@redhat.com>,
        Liran Alon <liran.alon@oracle.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        "H . Peter Anvin" <hpa@zytor.com>, x86@kernel.org,
        kvm@vger.kernel.org, linux-kernel@vger.kernel.org,
        syzbot+ded1696f6b50b615b630@syzkaller.appspotmail.com
Subject: Re: [RFC PATCH] kvm: x86/vmx: Use kzalloc for cached_vmcs12
Message-ID: <20190115175111.GB68985@google.com>
References: <6f79d9be-fa76-3a06-2612-f44f3a18ece7@redhat.com>
 <20190114234728.49239-1-tmroeder@google.com>
 <20190115024304.GD5141@linux.intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20190115024304.GD5141@linux.intel.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Mon, Jan 14, 2019 at 06:43:04PM -0800, Sean Christopherson wrote:
> On Mon, Jan 14, 2019 at 03:47:28PM -0800, Tom Roeder wrote:
> > This changes the allocation of cached_vmcs12 to use kzalloc instead of
> > kmalloc. This removes the information leak found by Syzkaller (see
> > Reported-by) in this case and prevents similar leaks from happening
> > based on cached_vmcs12.
> 
> Is the leak specific to vmx_set_nested_state(), e.g. can we zero out
> the memory if copy_from_user() fails instead of taking the hit on every
> allocation?

I don't know if the leak is specific to vmx_set_nested_state.

This question (and the rest of the thread from November) goes to the
heart of what I wanted to get feedback about; hence the "RFC" part of
the subject line.

I'm new to the kernel, and I don't know all the idioms and expectations,
so the follow analysis is an outsider's view of the issue at hand.

What I see in this case is a field that is intended to be copied to the
guest and is allocated initially with data from the kernel. I'm sure we
could figure out all the current paths and error cases that we need to
handle to make sure that this data never leaks. Future reviewers then
also need to make sure that changes to the nested VMX code don't leak
data from this field.

Why not instead make sure that there isn't any data to leak in the first
place?

I understand that there's a cost to kzalloc vs. kmalloc, but I don't
know what it is in practice; slab.c shows that the extra code for the
__GFP_ZERO flag is a memset of 0 over the allocated memory. The
allocation looks very infrequent for the two lines in this patch, since
they occur in enter_vmx_operation. That sounds to me like the allocation
only happens when the guest enables nested virtualization.

Given the frequency of allocation and the relative security benefit of
not having to worry about leaking the data, I'd advocate for changing it
here.