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:   Wed, 06 Jul 2022 08:11:46 +0100
Message-ID: <87edyyn2a5.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: <m2h740zz1i.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>
        <87letd0wqm.wl-maz@kernel.org>
        <m2h740zz1i.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 15:22:51 +0100,
Schspa Shi <schspa@gmail.com> wrote:
> 
> 
> Marc Zyngier <maz@kernel.org> writes:
> 
> >> 
> >> 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.
> One example is
> int vgic_v3_has_attr_regs(struct kvm_device *dev, struct
> kvm_device_attr *attr)
> {
> 	...
>    case KVM_DEV_ARM_VGIC_GRP_CPU_SYSREGS: {
> 		u64 reg, id;
> 
> 		id = (attr->attr & KVM_DEV_ARM_VGIC_SYSREG_INSTR_MASK);
> 		return vgic_v3_has_cpu_sysregs_attr(vcpu, 0, id, &reg);
> 	}
> 
> }
> 
> The funcion vgic_v3_has_cpu_sysregs_attr will read reg's value to
> params without initialization. There should have no problems,
> because the register value never used.

Thanks for pointing this out.

I spent some time looking at this, and this is only the tip of the
iceberg. The whole userspace interaction with the GIC sysregs is ugly
(at best), and needs some love.

I've written a small series[1] cleaning things up, which needs testing
(I've just checked that it was compiling correctly). I'd appreciate
you running your tool on it.

	M.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/log/?h=kvm-arm64/sysreg-cleanup-5.20

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