Re: [PATCH] KVM: arm64: Fix 64 bit mmio handle
From: Schspa Shi
Date: Wed Jul 06 2022 - 07:32:57 EST
Marc Zyngier <maz@xxxxxxxxxx> writes:
On Fri, 01 Jul 2022 15:22:51 +0100,
Schspa Shi <schspa@xxxxxxxxx> wrote:
Marc Zyngier <maz@xxxxxxxxxx> 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, ®);
}
}
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.
I have run static code analysis software upon this branch, and the
warnings have gone.
M.
[1]
https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/log/?h=kvm-arm64/sysreg-cleanup-5.20
--
BRs
Schspa Shi