linux-kernel - Re: [PATCH v2] KVM: x86: Fix poll command

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: <20230418075923.752113-1-alexjlzheng@tencent.com>
Date:   Tue, 18 Apr 2023 15:59:23 +0800
From:   alexjlzheng@...il.com
To:     seanjc@...gle.com
Cc:     alexjlzheng@...il.com, alexjlzheng@...cent.com, bp@...en8.de,
        dave.hansen@...ux.intel.com, hpa@...or.com, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org, mingo@...hat.com,
        pbonzini@...hat.com, tglx@...utronix.de, x86@...nel.org
Subject: Re: [PATCH v2] KVM: x86: Fix poll command

On Mon, 17 Apr 2023, Sean Christopherson <seanjc@...gle.com> wrote:
> On Sat, Apr 15, 2023, alexjlzheng@...il.com wrote:
> > On Fri, 14 Apr 2023, Sean Christopherson <seanjc@...gle.com> wrote:
> > > On Thu, Apr 13, 2023, alexjlzheng@...il.com wrote:
> > > > Fix the implementation of pic_poll_read():
> > > > 1. Set Bit 7 when there is an interrupt
> > > > 2. Return 0 when there is no interrupt
> > > 
> > > I don't think #2 is justified.  The spec says:
> > > 
> > >   The interrupt requests are ordered in priority from 0 through 7 (0 highest).
> > 
> > This is only true when don't use rotation for priority or just reset the 8259a.
> > It's prossible to change priorities, i.e. Specific Rotation Mode or Automatic
> > Rotation Mode.
> > 
> > > 
> > > I.e. the current code enumerates the _lowest_ priority when there is no interrupt,
> > > which seems more correct than reporting the highest priority possible.
> > 
> > The practice and interpretation of returning to the lowest priority interrupt
> > when there are no active interrupts in the PIC doesn't seem reasonable, as far as I
> > understand. For #2, in my opinion, the correct interpretation of the current code
> > may be that a spurious interrupt is returned(IRQ 7 is used for that according to
> > the 8259 hardware manual).
> > 
> > For #2, the main purpose of returning 0 is to set Bit 7 of the return value to 0
> > to indicate that there is no interrupt.
> 
> Is there an actual real world chunk of guest code that is broken by KVM's behavior
> for the "no interrupt" case?  Because if not, my strong preference is to leave the
> code as-is.
> 
> I have no objection to setting bit 7 when there is an interrupt, as that behavior
> is explicitly called out and KVM is clearly in the wrong.

Very happy that we have reached a consensus on #1.

> 
> But for the "no interrupt" case, there are a lot of "mays" and "seems" in both of
> our responses, i.e. it's not obvious that the current code is outright wrong, nor
> that it is correct either.  Given the lack of clarity, unless there's a guest that's
> actually broken by KVM's current implementation, I see no benefit to changing KVM's
> behavior, only the potential for breaking existing KVM guests.

For #2, neither returning 0 nor 7 will affect the behavior of interrupt handling in
the guest os. Because their Bit 7 are all 0, the guest os will interpret them as no
interrupt. However, keeping it as it is (return 7) will reduce the readability of
the pic_poll_read() code. When developers compare the code in kvm_pic_read_irq(),
they may think that what is returned in #2 is a spurious interrupt, but this is not.

> 
> And if the "no interrupt" case really does need to be fixed, please split it to
> a separate patch.

For the reasons above, I suggest fix #2. I will split it to a separate patch.

Thank you.
Jinliang Zheng