linux-kernel - Re: [PATCH 1/2] KVM: X86: Move ignore

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: <20200710045819.GB24919@linux.intel.com>
Date:   Thu, 9 Jul 2020 21:58:19 -0700
From:   Sean Christopherson <sean.j.christopherson@...el.com>
To:     Paolo Bonzini <pbonzini@...hat.com>
Cc:     Peter Xu <peterx@...hat.com>, linux-kernel@...r.kernel.org,
        kvm@...r.kernel.org, Vitaly Kuznetsov <vkuznets@...hat.com>
Subject: Re: [PATCH 1/2] KVM: X86: Move ignore_msrs handling upper the stack

On Fri, Jul 10, 2020 at 12:11:54AM +0200, Paolo Bonzini wrote:
> On 09/07/20 23:50, Peter Xu wrote:
> >> Sean: Objection your honor.
> >> Paolo: Overruled, you're wrong.
> >> Sean: Phooey.
> >>
> >> My point is that even though I still object to this series, Paolo has final
> >> say.
> >
> > I could be wrong, but I feel like Paolo was really respecting your input, as
> > always.
> 
> I do respect Sean's input

Ya, my comments were in jest.  Sorry if I implied I was grumpy about Paolo
taking this patch, because I'm not.  Just stubborn :-)

> but I also believe that in this case there's three questions:
> 
> a) should KVM be allowed to use the equivalent of rdmsr*_safe() on guest
> MSRs?  I say a mild yes, Sean says a strong no.

It's more that I don't think host_initiated=true is the equivalent of
rdmsr_safe().  It kind of holds true for rdmsr, but that's most definitely
not the case for wrmsr where host_initiated=true completely changes what
is/isn't allow.  And if using host_initiated=true for rdmsr is allowed,
then logically using it for wrmsr is also allowed.

> b) is it good to separate the "1" and "-EINVAL" results so that
> ignore_msrs handling can be moved out of the MSR access functions?  I
> say yes because KVM should never rely on ignore_msrs; Sean didn't say
> anything (it's not too relevant if you answer no to the first question).
> 
> c) is it possible to reimplement TSX_CTRL_MSR to avoid using the
> equivalent of rdmsr*_safe()?  Sean says yes and it's not really possible
> to argue against that, but then it doesn't really matter if you answer
> yes to the first two questions.
> 
> Sean sees your patch mostly as answering "yes" to the question (a), and
> therefore disagrees with it.  I see your patch mostly as answering "yes"
> to question (b), and therefore like it.  I would also accept a patch
> that reimplements TSX_CTRL_MSR (question c), but I consider your patch
> to be an improvement anyway (question b).
> 
> > It's just as simple as a 2:1 vote, isn't it? (I can still count myself
> > in for the vote, right? :)
> 
> I do have the final say but I try to use that as little as possible (or
> never).  And then it happens that ever so rare disagreements cluster in
> the same week!
> 
> The important thing is to analyze the source of the disagreement.
> Usually when that happens, it's because a change has multiple purposes
> and people see it in a different way.
> 
> In this case, I'm happy to accept this patch (and overrule Sean) not
> because he's wrong on question (a), but because in my opinion the actual
> motivation of the patch is question (b).
> 
> To be fair, I would prefer it if ignore_msrs didn't apply to
> host-initiated MSR accesses at all (only guest accesses).  That would
> make this series much much simpler.  It wouldn't solve the disagremement
> on question (a), but perhaps it would be a patch that Sean would agree on.

I think I could get behind that.  It shoudn't interfere with my crusade to
vanquish host_initiated :-)