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

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

[Schspa Shi]

If the len is 8 bytes, we can't get the correct sign extend for
be system.

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)) {
 			mask = 1U << ((len * 8) - 1);
 			data = (data ^ mask) - mask;
 		}
-- 
2.37.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Marc Zyngier]

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.

I must be missing something. And how is that related to running BE? BE
in the host? The guest?

Please convince me.

	M.

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Schspa Shi]

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.

>
> Please convince me.
>
> 	M.

-- 
BRs
Schspa Shi

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Marc Zyngier]

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.

Thanks,

          M.
-- 
Jazz is not dead. It just smells funny...

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Schspa Shi]

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?

> Thanks,
>
>          M.


-- 
Schspa Shi
BRs

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Marc Zyngier]

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.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Schspa Shi]

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.

-- 
BRs
Schspa Shi

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Marc Zyngier]

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.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Schspa Shi]

Marc Zyngier <maz@kernel.org> writes:

> 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.
>

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel