diff mbox

MIPS: KVM: do not sign extend on unsigned MMIO load

Message ID 1431002870-30098-1-git-send-email-hofrat@osadl.org (mailing list archive)
State New, archived
Headers show

Commit Message

Nicholas Mc Guire May 7, 2015, 12:47 p.m. UTC
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 <hofrat@osadl.org>
---

Thanks to James Hogan <james.hogan@imgtec.com> 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(-)

Comments

James Hogan May 8, 2015, 2:16 p.m. UTC | #1
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 <hofrat@osadl.org>

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

Reviewed-by: James Hogan <james.hogan@imgtec.com>
Tested-by: James Hogan <james.hogan@imgtec.com>

It looks suitable for stable too (3.10+).

Cheers
James

> ---
> 
> Thanks to James Hogan <james.hogan@imgtec.com> 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:
>
James Hogan June 8, 2015, 8:33 a.m. UTC | #2
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 <hofrat@osadl.org>
> 
> Looks good to me. I wrote an MMIO test to reproduce the issue, and this
> fixes it.
> 
> Reviewed-by: James Hogan <james.hogan@imgtec.com>
> Tested-by: James Hogan <james.hogan@imgtec.com>
> 
> 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 <james.hogan@imgtec.com> 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:
>>
>
Jiri Slaby June 10, 2015, 12:57 p.m. UTC | #3
-----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 <hofrat@osadl.org>
>> 
>> Looks good to me. I wrote an MMIO test to reproduce the issue,
>> and this fixes it.
>> 
>> Reviewed-by: James Hogan <james.hogan@imgtec.com> Tested-by:
>> James Hogan <james.hogan@imgtec.com>
>> 
>> 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-----
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Luis Henriques June 12, 2015, 12:07 p.m. UTC | #4
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 <hofrat@osadl.org>
> > 
> > Looks good to me. I wrote an MMIO test to reproduce the issue, and this
> > fixes it.
> > 
> > Reviewed-by: James Hogan <james.hogan@imgtec.com>
> > Tested-by: James Hogan <james.hogan@imgtec.com>
> > 
> > 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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Greg KH June 19, 2015, 7:14 p.m. UTC | #5
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 <hofrat@osadl.org>
> > 
> > Looks good to me. I wrote an MMIO test to reproduce the issue, and this
> > fixes it.
> > 
> > Reviewed-by: James Hogan <james.hogan@imgtec.com>
> > Tested-by: James Hogan <james.hogan@imgtec.com>
> > 
> > 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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
diff mbox

Patch

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: