Re: [PATCH] KVM: arm64: Fix 64 bit mmio handle

[Marc Zyngier <maz@kernel.org>]

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.