Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 01 Jul 2022 14:48:49 +0100
Message-ID: <87letd0wqm.wl-maz@kernel.org>
From:   Marc Zyngier <maz@kernel.org>
To:     Schspa Shi <schspa@gmail.com>
Cc:     james.morse@arm.com, alexandru.elisei@arm.com,
        suzuki.poulose@arm.com, catalin.marinas@arm.com, will@kernel.org,
        linux-arm-kernel@lists.infradead.org, kvmarm@lists.cs.columbia.edu,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] KVM: arm64: Fix 64 bit mmio handle
In-Reply-To: <m2sfnlvwup.fsf@gmail.com>
References: <20220630161220.53449-1-schspa@gmail.com>
        <87mtdu15ok.wl-maz@kernel.org>
        <m2bkua148a.fsf@gmail.com>
        <a5ca4db3db9ef101258cab94d6b7e045@kernel.org>
        <m2sfnlvwup.fsf@gmail.com>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue)
 FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/27.1
 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO)
MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue")
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

On Fri, 01 Jul 2022 13:22:21 +0100,
Schspa Shi <schspa@gmail.com> wrote:
> 
> 
> Marc Zyngier <maz@kernel.org> writes:
> 
> > On 2022-06-30 17:50, Schspa Shi wrote:
> >> Marc Zyngier <maz@kernel.org> writes:
> >> 
> >>> On Thu, 30 Jun 2022 17:12:20 +0100,
> >>> Schspa Shi <schspa@gmail.com> wrote:
> >>>> If the len is 8 bytes, we can't get the correct sign extend for
> >>>> be system.
> >>> I'm afraid you'll have to give me a bit more details.
> >>> 
> >>>> Fix the mask type len and the comparison of length.
> >>>> Signed-off-by: Schspa Shi <schspa@gmail.com>
> >>>> ---
> >>>>  arch/arm64/kvm/mmio.c | 4 ++--
> >>>>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>>> diff --git a/arch/arm64/kvm/mmio.c b/arch/arm64/kvm/mmio.c
> >>>> index 3dd38a151d2a6..0692f8b18f35c 100644
> >>>> --- a/arch/arm64/kvm/mmio.c
> >>>> +++ b/arch/arm64/kvm/mmio.c
> >>>> @@ -81,8 +81,8 @@ unsigned long kvm_mmio_read_buf(const void
> >>>> *buf, unsigned
> >>>> int len)
> >>>>  int kvm_handle_mmio_return(struct kvm_vcpu *vcpu)
> >>>>  {
> >>>>  	unsigned long data;
> >>>> +	unsigned long mask;
> >>>>  	unsigned int len;
> >>>> -	int mask;
> >>>>   	/* Detect an already handled MMIO return */
> >>>>  	if (unlikely(!vcpu->mmio_needed))
> >>>> @@ -97,7 +97,7 @@ int kvm_handle_mmio_return(struct kvm_vcpu
> >>>> *vcpu)
> >>>>  		data = kvm_mmio_read_buf(run->mmio.data, len);
> >>>>   		if (kvm_vcpu_dabt_issext(vcpu) &&
> >>>> -		    len < sizeof(unsigned long)) {
> >>>> +		    len <= sizeof(unsigned long)) {
> >>> If you're reading an 8 byte quantity, what is there to
> >>> sign-extend?
> >>> Sign extension only makes sense if what you're reading is
> >>> *smaller*
> >>> than the size of the register you are targeting.
> >>> 
> >> Yes, you are correct, sorry for my bad patch.
> >> Please ignore this patch.
> >> 
> >>> I must be missing something. And how is that related to running
> >>> BE? BE
> >>> in the host? The guest?
> >> I mean BE is for guest running with BE mode.
> > 
> > So what problem did you see? If you have noticed something going
> > wrong, I'd like to get it fixed.
> > 
> 
> I have running some static code analysis software upon Kernel code.
> Seeing there is possible overflow.
> 
> maks << 1U << ((len * 8) -1);
> 
> The AI don't know, len is only the value of 1, 2, 4, and make this
> a warnings
> 
> I tring to analysis this, but didn't realize the real scenario of
> sign extension, and finally sent this problematic patch.
> 
> I do see some uninitialized memory reads (the values are not used
> in the end, just as temporary space for API execution),
> do we need to fix these?

You need to be more descriptive here. What uninitialised reads? In
general, pointing at the code and providing a full description of what
you think is incorrect would really help...

	M.

-- 
Without deviation from the norm, progress is not possible.