2015-05-07 12:56:11

by Nicholas Mc Guire

[permalink] [raw]
Subject: [PATCH] MIPS: KVM: do not sign extend on unsigned MMIO load

Fix possible unintended sign extension in unsigned MMIO loads by casting
to uint16_t in the case of mmio_needed != 2.

Signed-off-by: Nicholas Mc Guire <[email protected]>
---

Thanks to James Hogan <[email protected]> for the explaination of
mmio_needed (there is not really any helpful comment in the code on this)
in this case (mmio_needed!=2) it should be unsigned.

Patch was only compile tested msp71xx_defconfig + CONFIG_KVM=m

Patch is against 4.1-rc2 (localversion-next is -next-20150506)

arch/mips/kvm/emulate.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/mips/kvm/emulate.c b/arch/mips/kvm/emulate.c
index 6230f37..2f0fc60 100644
--- a/arch/mips/kvm/emulate.c
+++ b/arch/mips/kvm/emulate.c
@@ -2415,7 +2415,7 @@ enum emulation_result kvm_mips_complete_mmio_load(struct kvm_vcpu *vcpu,
if (vcpu->mmio_needed == 2)
*gpr = *(int16_t *) run->mmio.data;
else
- *gpr = *(int16_t *) run->mmio.data;
+ *gpr = *(uint16_t *)run->mmio.data;

break;
case 1:
--
1.7.10.4


2015-05-08 14:17:34

by James Hogan

[permalink] [raw]
Subject: Re: [PATCH] MIPS: KVM: do not sign extend on unsigned MMIO load

On 07/05/15 13:47, Nicholas Mc Guire wrote:
> Fix possible unintended sign extension in unsigned MMIO loads by casting
> to uint16_t in the case of mmio_needed != 2.
>
> Signed-off-by: Nicholas Mc Guire <[email protected]>

Looks good to me. I wrote an MMIO test to reproduce the issue, and this
fixes it.

Reviewed-by: James Hogan <[email protected]>
Tested-by: James Hogan <[email protected]>

It looks suitable for stable too (3.10+).

Cheers
James

> ---
>
> Thanks to James Hogan <[email protected]> for the explaination of
> mmio_needed (there is not really any helpful comment in the code on this)
> in this case (mmio_needed!=2) it should be unsigned.
>
> Patch was only compile tested msp71xx_defconfig + CONFIG_KVM=m
>
> Patch is against 4.1-rc2 (localversion-next is -next-20150506)
>
> arch/mips/kvm/emulate.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/mips/kvm/emulate.c b/arch/mips/kvm/emulate.c
> index 6230f37..2f0fc60 100644
> --- a/arch/mips/kvm/emulate.c
> +++ b/arch/mips/kvm/emulate.c
> @@ -2415,7 +2415,7 @@ enum emulation_result kvm_mips_complete_mmio_load(struct kvm_vcpu *vcpu,
> if (vcpu->mmio_needed == 2)
> *gpr = *(int16_t *) run->mmio.data;
> else
> - *gpr = *(int16_t *) run->mmio.data;
> + *gpr = *(uint16_t *)run->mmio.data;
>
> break;
> case 1:
>


Attachments:
signature.asc (819.00 B)
OpenPGP digital signature

2015-06-08 08:34:17

by James Hogan

[permalink] [raw]
Subject: Re: [PATCH] MIPS: KVM: do not sign extend on unsigned MMIO load

Hi stable folk,

On 08/05/15 15:16, James Hogan wrote:
> On 07/05/15 13:47, Nicholas Mc Guire wrote:
>> Fix possible unintended sign extension in unsigned MMIO loads by casting
>> to uint16_t in the case of mmio_needed != 2.
>>
>> Signed-off-by: Nicholas Mc Guire <[email protected]>
>
> Looks good to me. I wrote an MMIO test to reproduce the issue, and this
> fixes it.
>
> Reviewed-by: James Hogan <[email protected]>
> Tested-by: James Hogan <[email protected]>
>
> It looks suitable for stable too (3.10+).

This has reached mainline, commit ed9244e6c534612d2b5ae47feab2f55a0d4b4ced

Please could it be added to stable (3.10+).

Thanks
James


>
> Cheers
> James
>
>> ---
>>
>> Thanks to James Hogan <[email protected]> for the explaination of
>> mmio_needed (there is not really any helpful comment in the code on this)
>> in this case (mmio_needed!=2) it should be unsigned.
>>
>> Patch was only compile tested msp71xx_defconfig + CONFIG_KVM=m
>>
>> Patch is against 4.1-rc2 (localversion-next is -next-20150506)
>>
>> arch/mips/kvm/emulate.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/mips/kvm/emulate.c b/arch/mips/kvm/emulate.c
>> index 6230f37..2f0fc60 100644
>> --- a/arch/mips/kvm/emulate.c
>> +++ b/arch/mips/kvm/emulate.c
>> @@ -2415,7 +2415,7 @@ enum emulation_result kvm_mips_complete_mmio_load(struct kvm_vcpu *vcpu,
>> if (vcpu->mmio_needed == 2)
>> *gpr = *(int16_t *) run->mmio.data;
>> else
>> - *gpr = *(int16_t *) run->mmio.data;
>> + *gpr = *(uint16_t *)run->mmio.data;
>>
>> break;
>> case 1:
>>
>


Attachments:
signature.asc (819.00 B)
OpenPGP digital signature

2015-06-10 12:57:12

by Jiri Slaby

[permalink] [raw]
Subject: Re: [PATCH] MIPS: KVM: do not sign extend on unsigned MMIO load

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

On 06/08/2015, 10:33 AM, James Hogan wrote:
> Hi stable folk,
>
> On 08/05/15 15:16, James Hogan wrote:
>> On 07/05/15 13:47, Nicholas Mc Guire wrote:
>>> Fix possible unintended sign extension in unsigned MMIO loads
>>> by casting to uint16_t in the case of mmio_needed != 2.
>>>
>>> Signed-off-by: Nicholas Mc Guire <[email protected]>
>>
>> Looks good to me. I wrote an MMIO test to reproduce the issue,
>> and this fixes it.
>>
>> Reviewed-by: James Hogan <[email protected]> Tested-by:
>> James Hogan <[email protected]>
>>
>> It looks suitable for stable too (3.10+).
>
> This has reached mainline, commit
> ed9244e6c534612d2b5ae47feab2f55a0d4b4ced
>
> Please could it be added to stable (3.10+).

Applied to 3.12. Thanks.

- --
js
suse labs
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQIcBAEBCAAGBQJVeDQcAAoJEL0lsQQGtHBJ7q4P/3Q7y1FHwPKDhsdIdyyRypR6
OXaH/6eNzpBhvSngP1gnx9MiyESTYihFVlRJsV6hYYzRcippnU0BP88dx9ntYrc1
Accbhj/PPYcMqfCnYdL80Kxt9EomeuxEDcCdbp8twnReTt44xNAGHePiNh9GrhjG
VKBMralyyjymtwyamCGb2W2aLNhxELIG3gXJTb7Q7E071LVeqQA6g+VNQ2QHwFYq
FkJexePsLu/j3zVxH+rsQPJA6E1oKfUJb3jQHAtZHAH95Um0r8T4jUVSgFhyk3r6
9tlkazL3P8Iui6lxbrV1vNCPAhhucY7PmX99uGhdroKJOKDCDPsVOKyJbxeHrUBR
3zrMpB9x2uXd6WpDLDfL+bI8bCG6NVXZPGgSd7P+r/UbNuZ6VBNSVdqlWUeoMWGR
ZS+HFMxVOiNplYudCTdLbJDLhm2XCWeW2lqszll/8Nk1c1FZkl8YbgpmdXfutKeU
LQfUfS4tr0AQ7BqXf3bPUGrSGZO7e1V5R4gAa+Yqo6ZjDOj20AjYvs0oW4ubgLg8
OJrHcJDLkEKrMDIZ7qpRZxyz56yrOgcfVbYB1fudXaV+e38t+kO0sujdNSJnHK8h
T3kfa96QW2gOi7Cys1o2OaQboY2wFxK3/YefX3Jn+N7tGedKUwF4IYHtj17YqX1/
8BkHSZZ9HQsJqyRAXBux
=qlvA
-----END PGP SIGNATURE-----

2015-06-12 12:07:51

by Luis Henriques

[permalink] [raw]
Subject: Re: [PATCH] MIPS: KVM: do not sign extend on unsigned MMIO load

On Mon, Jun 08, 2015 at 09:33:50AM +0100, James Hogan wrote:
> Hi stable folk,
>
> On 08/05/15 15:16, James Hogan wrote:
> > On 07/05/15 13:47, Nicholas Mc Guire wrote:
> >> Fix possible unintended sign extension in unsigned MMIO loads by casting
> >> to uint16_t in the case of mmio_needed != 2.
> >>
> >> Signed-off-by: Nicholas Mc Guire <[email protected]>
> >
> > Looks good to me. I wrote an MMIO test to reproduce the issue, and this
> > fixes it.
> >
> > Reviewed-by: James Hogan <[email protected]>
> > Tested-by: James Hogan <[email protected]>
> >
> > It looks suitable for stable too (3.10+).
>
> This has reached mainline, commit ed9244e6c534612d2b5ae47feab2f55a0d4b4ced
>
> Please could it be added to stable (3.10+).
>
> Thanks
> James

Thanks, I'm queuing it for the 3.16 as well.

Cheers,
--
Lu?s

2015-06-19 19:14:18

by Greg KH

[permalink] [raw]
Subject: Re: [PATCH] MIPS: KVM: do not sign extend on unsigned MMIO load

On Mon, Jun 08, 2015 at 09:33:50AM +0100, James Hogan wrote:
> Hi stable folk,
>
> On 08/05/15 15:16, James Hogan wrote:
> > On 07/05/15 13:47, Nicholas Mc Guire wrote:
> >> Fix possible unintended sign extension in unsigned MMIO loads by casting
> >> to uint16_t in the case of mmio_needed != 2.
> >>
> >> Signed-off-by: Nicholas Mc Guire <[email protected]>
> >
> > Looks good to me. I wrote an MMIO test to reproduce the issue, and this
> > fixes it.
> >
> > Reviewed-by: James Hogan <[email protected]>
> > Tested-by: James Hogan <[email protected]>
> >
> > It looks suitable for stable too (3.10+).
>
> This has reached mainline, commit ed9244e6c534612d2b5ae47feab2f55a0d4b4ced
>
> Please could it be added to stable (3.10+).

It does not apply to 3.10 or 3.14-stable, so please provide a backport
if you want it there.

thanks,

greg k-h

2015-07-08 14:26:11

by James Hogan

[permalink] [raw]
Subject: [PATCH stable <3.17] MIPS: KVM: Do not sign extend on unsigned MMIO load

From: Nicholas Mc Guire <[email protected]>

commit ed9244e6c534612d2b5ae47feab2f55a0d4b4ced upstream.

Fix possible unintended sign extension in unsigned MMIO loads by casting
to uint16_t in the case of mmio_needed != 2.

Signed-off-by: Nicholas Mc Guire <[email protected]>
Reviewed-by: James Hogan <[email protected]>
Tested-by: James Hogan <[email protected]>
Cc: Gleb Natapov <[email protected]>
Cc: Paolo Bonzini <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Patchwork: https://patchwork.linux-mips.org/patch/9985/
Signed-off-by: Ralf Baechle <[email protected]>
Cc: <[email protected]> # v3.10+
Signed-off-by: James Hogan <[email protected]>
---
This is a trivial backport (i.e. git cherry-pick, git format-patch) for
stable branches before v3.17, due to the commit d7d5b05faf16 ("MIPS:
KVM: Rename files to remove the prefix "kvm_" and "kvm_mips_"") which
renamed a bunch of files including this one.
---
arch/mips/kvm/kvm_mips_emul.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/mips/kvm/kvm_mips_emul.c b/arch/mips/kvm/kvm_mips_emul.c
index e75ef8219caf..c76f297b7149 100644
--- a/arch/mips/kvm/kvm_mips_emul.c
+++ b/arch/mips/kvm/kvm_mips_emul.c
@@ -1626,7 +1626,7 @@ kvm_mips_complete_mmio_load(struct kvm_vcpu *vcpu, struct kvm_run *run)
if (vcpu->mmio_needed == 2)
*gpr = *(int16_t *) run->mmio.data;
else
- *gpr = *(int16_t *) run->mmio.data;
+ *gpr = *(uint16_t *)run->mmio.data;

break;
case 1:
--
2.3.6