diff mbox series

KVM: PPC: fix oops when checking KVM_CAP_PPC_HTM

Message ID 150542618501.6859.11512107352972110416.stgit@bahia.lan
State Accepted
Headers show
Series KVM: PPC: fix oops when checking KVM_CAP_PPC_HTM | expand

Commit Message

Greg Kurz Sept. 14, 2017, 9:56 p.m. UTC 
The following program causes a kernel oops:

#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <sys/ioctl.h>
#include <linux/kvm.h>

main()
{
    int fd = open("/dev/kvm", O_RDWR);
    ioctl(fd, KVM_CHECK_EXTENSION, KVM_CAP_PPC_HTM);
}

This happens because when using the global KVM fd with
KVM_CHECK_EXTENSION, kvm_vm_ioctl_check_extension() gets
called with a NULL kvm argument, which gets dereferenced
in is_kvmppc_hv_enabled(). Spotted while reading the code.

Let's use the hv_enabled fallback variable, like everywhere
else in this function.

Fixes: 23528bb21ee2 ("KVM: PPC: Introduce KVM_CAP_PPC_HTM")
Cc: stable@vger.kernel.org # v4.7+
Signed-off-by: Greg Kurz <groug@kaod.org>
---
 arch/powerpc/kvm/powerpc.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)


--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Greg Kurz Sept. 15, 2017, 5:52 a.m. UTC | #1 
Dang! The mail relay at OVH has blacklisted Paul's address :-\

<paulus@samba.org>: host smtp.samba.org[144.76.82.148] said: 550-blacklisted at
    zen.spamhaus.org 550 https://www.spamhaus.org/sbl/query/SBL370982 (in reply
    to RCPT TO command)

Cc'ing Paul at ozlabs.org

On Fri, 15 Sep 2017 10:48:39 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Thu, Sep 14, 2017 at 11:56:25PM +0200, Greg Kurz wrote:
> > The following program causes a kernel oops:
> > 
> > #include <sys/types.h>
> > #include <sys/stat.h>
> > #include <fcntl.h>
> > #include <sys/ioctl.h>
> > #include <linux/kvm.h>
> > 
> > main()
> > {
> >     int fd = open("/dev/kvm", O_RDWR);
> >     ioctl(fd, KVM_CHECK_EXTENSION, KVM_CAP_PPC_HTM);
> > }
> > 
> > This happens because when using the global KVM fd with
> > KVM_CHECK_EXTENSION, kvm_vm_ioctl_check_extension() gets
> > called with a NULL kvm argument, which gets dereferenced
> > in is_kvmppc_hv_enabled(). Spotted while reading the code.
> > 
> > Let's use the hv_enabled fallback variable, like everywhere
> > else in this function.
> > 
> > Fixes: 23528bb21ee2 ("KVM: PPC: Introduce KVM_CAP_PPC_HTM")
> > Cc: stable@vger.kernel.org # v4.7+
> > Signed-off-by: Greg Kurz <groug@kaod.org>  
> 
> I don't think this is right.  I'm pretty sure you want to fall back to
> hv_enabled *only when* kvm is NULL.  Otherwise if you have a PR guest
> on an HV capable machine, this will give the wrong answer, when called
> for that specific VM.
> 

Hmmm... this is what we get with this patch applied:

open("/dev/kvm", O_RDWR)                = 3
ioctl(3, KVM_CHECK_EXTENSION, 0x84)     = 1 <== if HV is present
ioctl(3, KVM_CREATE_VM, 0x1)            = 4 <== HV
ioctl(4, KVM_CHECK_EXTENSION, 0x84)     = 1
ioctl(3, KVM_CREATE_VM, 0x2)            = 5 <== PR
ioctl(5, KVM_CHECK_EXTENSION, 0x84)     = 0

The hv_enabled variable is set as follows:

	/* Assume we're using HV mode when the HV module is loaded */
	int hv_enabled = kvmppc_hv_ops ? 1 : 0;

	if (kvm) {
		/*
		 * Hooray - we know which VM type we're running on. Depend on
		 * that rather than the guess above.
		 */
		hv_enabled = is_kvmppc_hv_enabled(kvm);
	}

so we're good. :)

The last sentence in the commit message is maybe^wprobably not comprehensive
enough...

What about the following ?

The hv_enabled variable is initialized to 1 if HV is loaded or 0 otherwise.
In the case KVM_CHECK_EXTENSION is used with a VM fd, hv_enabled is updated
to is_kvmppc_hv_enabled(kvm). Let's use it here, like everywhere else in this
function.

> > ---
> >  arch/powerpc/kvm/powerpc.c |    3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> > 
> > diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
> > index 3480faaf1ef8..ee279c7f4802 100644
> > --- a/arch/powerpc/kvm/powerpc.c
> > +++ b/arch/powerpc/kvm/powerpc.c
> > @@ -644,8 +644,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
> >  		break;
> >  #endif
> >  	case KVM_CAP_PPC_HTM:
> > -		r = cpu_has_feature(CPU_FTR_TM_COMP) &&
> > -		    is_kvmppc_hv_enabled(kvm);
> > +		r = cpu_has_feature(CPU_FTR_TM_COMP) && hv_enabled;
> >  		break;
> >  	default:
> >  		r = 0;
> >   
>
Greg Kurz Sept. 15, 2017, 6:54 a.m. UTC | #2 
On Fri, 15 Sep 2017 07:52:49 +0200
Greg Kurz <groug@kaod.org> wrote:

> Dang! The mail relay at OVH has blacklisted Paul's address :-\
> 
> <paulus@samba.org>: host smtp.samba.org[144.76.82.148] said: 550-blacklisted at
>     zen.spamhaus.org 550 https://www.spamhaus.org/sbl/query/SBL370982 (in reply
>     to RCPT TO command)
> 

Dumb me! It's the opposite... OVH is blacklisted by smtp.samba.org :-\

Sigh.
David Gibson Sept. 15, 2017, 8:59 a.m. UTC | #3 
On Fri, Sep 15, 2017 at 07:52:49AM +0200, Greg Kurz wrote:
> Dang! The mail relay at OVH has blacklisted Paul's address :-\
> 
> <paulus@samba.org>: host smtp.samba.org[144.76.82.148] said: 550-blacklisted at
>     zen.spamhaus.org 550 https://www.spamhaus.org/sbl/query/SBL370982 (in reply
>     to RCPT TO command)
> 
> Cc'ing Paul at ozlabs.org
> 
> On Fri, 15 Sep 2017 10:48:39 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Thu, Sep 14, 2017 at 11:56:25PM +0200, Greg Kurz wrote:
> > > The following program causes a kernel oops:
> > > 
> > > #include <sys/types.h>
> > > #include <sys/stat.h>
> > > #include <fcntl.h>
> > > #include <sys/ioctl.h>
> > > #include <linux/kvm.h>
> > > 
> > > main()
> > > {
> > >     int fd = open("/dev/kvm", O_RDWR);
> > >     ioctl(fd, KVM_CHECK_EXTENSION, KVM_CAP_PPC_HTM);
> > > }
> > > 
> > > This happens because when using the global KVM fd with
> > > KVM_CHECK_EXTENSION, kvm_vm_ioctl_check_extension() gets
> > > called with a NULL kvm argument, which gets dereferenced
> > > in is_kvmppc_hv_enabled(). Spotted while reading the code.
> > > 
> > > Let's use the hv_enabled fallback variable, like everywhere
> > > else in this function.
> > > 
> > > Fixes: 23528bb21ee2 ("KVM: PPC: Introduce KVM_CAP_PPC_HTM")
> > > Cc: stable@vger.kernel.org # v4.7+
> > > Signed-off-by: Greg Kurz <groug@kaod.org>  
> > 
> > I don't think this is right.  I'm pretty sure you want to fall back to
> > hv_enabled *only when* kvm is NULL.  Otherwise if you have a PR guest
> > on an HV capable machine, this will give the wrong answer, when called
> > for that specific VM.
> > 
> 
> Hmmm... this is what we get with this patch applied:
> 
> open("/dev/kvm", O_RDWR)                = 3
> ioctl(3, KVM_CHECK_EXTENSION, 0x84)     = 1 <== if HV is present
> ioctl(3, KVM_CREATE_VM, 0x1)            = 4 <== HV
> ioctl(4, KVM_CHECK_EXTENSION, 0x84)     = 1
> ioctl(3, KVM_CREATE_VM, 0x2)            = 5 <== PR
> ioctl(5, KVM_CHECK_EXTENSION, 0x84)     = 0
> 
> The hv_enabled variable is set as follows:
> 
> 	/* Assume we're using HV mode when the HV module is loaded */
> 	int hv_enabled = kvmppc_hv_ops ? 1 : 0;
> 
> 	if (kvm) {
> 		/*
> 		 * Hooray - we know which VM type we're running on. Depend on
> 		 * that rather than the guess above.
> 		 */
> 		hv_enabled = is_kvmppc_hv_enabled(kvm);
> 	}
> 
> so we're good. :)

Oh, sorry, missed that bit.  In that case.

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>



> The last sentence in the commit message is maybe^wprobably not comprehensive
> enough...
> 
> What about the following ?
> 
> The hv_enabled variable is initialized to 1 if HV is loaded or 0 otherwise.
> In the case KVM_CHECK_EXTENSION is used with a VM fd, hv_enabled is updated
> to is_kvmppc_hv_enabled(kvm). Let's use it here, like everywhere else in this
> function.
> 
> > > ---
> > >  arch/powerpc/kvm/powerpc.c |    3 +--
> > >  1 file changed, 1 insertion(+), 2 deletions(-)
> > > 
> > > diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
> > > index 3480faaf1ef8..ee279c7f4802 100644
> > > --- a/arch/powerpc/kvm/powerpc.c
> > > +++ b/arch/powerpc/kvm/powerpc.c
> > > @@ -644,8 +644,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
> > >  		break;
> > >  #endif
> > >  	case KVM_CAP_PPC_HTM:
> > > -		r = cpu_has_feature(CPU_FTR_TM_COMP) &&
> > > -		    is_kvmppc_hv_enabled(kvm);
> > > +		r = cpu_has_feature(CPU_FTR_TM_COMP) && hv_enabled;
> > >  		break;
> > >  	default:
> > >  		r = 0;
> > >   
> > 
>
Thomas Huth Sept. 18, 2017, 6:16 a.m. UTC | #4 
On 15.09.2017 10:59, David Gibson wrote:
> On Fri, Sep 15, 2017 at 07:52:49AM +0200, Greg Kurz wrote:
>> Dang! The mail relay at OVH has blacklisted Paul's address :-\
>>
>> <paulus@samba.org>: host smtp.samba.org[144.76.82.148] said: 550-blacklisted at
>>     zen.spamhaus.org 550 https://www.spamhaus.org/sbl/query/SBL370982 (in reply
>>     to RCPT TO command)
>>
>> Cc'ing Paul at ozlabs.org
>>
>> On Fri, 15 Sep 2017 10:48:39 +1000
>> David Gibson <david@gibson.dropbear.id.au> wrote:
>>
>>> On Thu, Sep 14, 2017 at 11:56:25PM +0200, Greg Kurz wrote:
>>>> The following program causes a kernel oops:
>>>>
>>>> #include <sys/types.h>
>>>> #include <sys/stat.h>
>>>> #include <fcntl.h>
>>>> #include <sys/ioctl.h>
>>>> #include <linux/kvm.h>
>>>>
>>>> main()
>>>> {
>>>>     int fd = open("/dev/kvm", O_RDWR);
>>>>     ioctl(fd, KVM_CHECK_EXTENSION, KVM_CAP_PPC_HTM);
>>>> }
>>>>
>>>> This happens because when using the global KVM fd with
>>>> KVM_CHECK_EXTENSION, kvm_vm_ioctl_check_extension() gets
>>>> called with a NULL kvm argument, which gets dereferenced
>>>> in is_kvmppc_hv_enabled(). Spotted while reading the code.
>>>>
>>>> Let's use the hv_enabled fallback variable, like everywhere
>>>> else in this function.
>>>>
>>>> Fixes: 23528bb21ee2 ("KVM: PPC: Introduce KVM_CAP_PPC_HTM")
>>>> Cc: stable@vger.kernel.org # v4.7+
>>>> Signed-off-by: Greg Kurz <groug@kaod.org>  
>>>
>>> I don't think this is right.  I'm pretty sure you want to fall back to
>>> hv_enabled *only when* kvm is NULL.  Otherwise if you have a PR guest
>>> on an HV capable machine, this will give the wrong answer, when called
>>> for that specific VM.
>>>
>>
>> Hmmm... this is what we get with this patch applied:
>>
>> open("/dev/kvm", O_RDWR)                = 3
>> ioctl(3, KVM_CHECK_EXTENSION, 0x84)     = 1 <== if HV is present
>> ioctl(3, KVM_CREATE_VM, 0x1)            = 4 <== HV
>> ioctl(4, KVM_CHECK_EXTENSION, 0x84)     = 1
>> ioctl(3, KVM_CREATE_VM, 0x2)            = 5 <== PR
>> ioctl(5, KVM_CHECK_EXTENSION, 0x84)     = 0
>>
>> The hv_enabled variable is set as follows:
>>
>> 	/* Assume we're using HV mode when the HV module is loaded */
>> 	int hv_enabled = kvmppc_hv_ops ? 1 : 0;
>>
>> 	if (kvm) {
>> 		/*
>> 		 * Hooray - we know which VM type we're running on. Depend on
>> 		 * that rather than the guess above.
>> 		 */
>> 		hv_enabled = is_kvmppc_hv_enabled(kvm);
>> 	}
>>
>> so we're good. :)
> 
> Oh, sorry, missed that bit.  In that case.
> 
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

LGTM, too:

Reviewed-by: Thomas Huth <thuth@redhat.com>
Michael Ellerman Oct. 12, 2017, 11:27 a.m. UTC | #5 
Greg Kurz <groug@kaod.org> writes:
> The following program causes a kernel oops:
>
> #include <sys/types.h>
> #include <sys/stat.h>
> #include <fcntl.h>
> #include <sys/ioctl.h>
> #include <linux/kvm.h>
>
> main()
> {
>     int fd = open("/dev/kvm", O_RDWR);
>     ioctl(fd, KVM_CHECK_EXTENSION, KVM_CAP_PPC_HTM);
> }
>
> This happens because when using the global KVM fd with
> KVM_CHECK_EXTENSION, kvm_vm_ioctl_check_extension() gets
> called with a NULL kvm argument, which gets dereferenced
> in is_kvmppc_hv_enabled(). Spotted while reading the code.
>
> Let's use the hv_enabled fallback variable, like everywhere
> else in this function.
>
> Fixes: 23528bb21ee2 ("KVM: PPC: Introduce KVM_CAP_PPC_HTM")
> Cc: stable@vger.kernel.org # v4.7+
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  arch/powerpc/kvm/powerpc.c |    3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
> index 3480faaf1ef8..ee279c7f4802 100644
> --- a/arch/powerpc/kvm/powerpc.c
> +++ b/arch/powerpc/kvm/powerpc.c
> @@ -644,8 +644,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
>  		break;
>  #endif
>  	case KVM_CAP_PPC_HTM:
> -		r = cpu_has_feature(CPU_FTR_TM_COMP) &&
> -		    is_kvmppc_hv_enabled(kvm);
> +		r = cpu_has_feature(CPU_FTR_TM_COMP) && hv_enabled;
>  		break;
>  	default:
>  		r = 0;

Did this go anywhere?

cheers
--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Greg Kurz Oct. 12, 2017, 12:51 p.m. UTC | #6 
On Thu, 12 Oct 2017 22:27:54 +1100
Michael Ellerman <mpe@ellerman.id.au> wrote:

> Greg Kurz <groug@kaod.org> writes:
> > The following program causes a kernel oops:
> >
> > #include <sys/types.h>
> > #include <sys/stat.h>
> > #include <fcntl.h>
> > #include <sys/ioctl.h>
> > #include <linux/kvm.h>
> >
> > main()
> > {
> >     int fd = open("/dev/kvm", O_RDWR);
> >     ioctl(fd, KVM_CHECK_EXTENSION, KVM_CAP_PPC_HTM);
> > }
> >
> > This happens because when using the global KVM fd with
> > KVM_CHECK_EXTENSION, kvm_vm_ioctl_check_extension() gets
> > called with a NULL kvm argument, which gets dereferenced
> > in is_kvmppc_hv_enabled(). Spotted while reading the code.
> >
> > Let's use the hv_enabled fallback variable, like everywhere
> > else in this function.
> >
> > Fixes: 23528bb21ee2 ("KVM: PPC: Introduce KVM_CAP_PPC_HTM")
> > Cc: stable@vger.kernel.org # v4.7+
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> >  arch/powerpc/kvm/powerpc.c |    3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
> > index 3480faaf1ef8..ee279c7f4802 100644
> > --- a/arch/powerpc/kvm/powerpc.c
> > +++ b/arch/powerpc/kvm/powerpc.c
> > @@ -644,8 +644,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
> >  		break;
> >  #endif
> >  	case KVM_CAP_PPC_HTM:
> > -		r = cpu_has_feature(CPU_FTR_TM_COMP) &&
> > -		    is_kvmppc_hv_enabled(kvm);
> > +		r = cpu_has_feature(CPU_FTR_TM_COMP) && hv_enabled;
> >  		break;
> >  	default:
> >  		r = 0;  
> 
> Did this go anywhere?
> 
> cheers

I'm afraid not... and I haven't tried to ping Paul yet, since he's
supposed to be on vacation from what I've been told.
--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Gibson Oct. 12, 2017, 10:20 p.m. UTC | #7 
On Thu, Oct 12, 2017 at 02:51:57PM +0200, Greg Kurz wrote:
> On Thu, 12 Oct 2017 22:27:54 +1100
> Michael Ellerman <mpe@ellerman.id.au> wrote:
> 
> > Greg Kurz <groug@kaod.org> writes:
> > > The following program causes a kernel oops:
> > >
> > > #include <sys/types.h>
> > > #include <sys/stat.h>
> > > #include <fcntl.h>
> > > #include <sys/ioctl.h>
> > > #include <linux/kvm.h>
> > >
> > > main()
> > > {
> > >     int fd = open("/dev/kvm", O_RDWR);
> > >     ioctl(fd, KVM_CHECK_EXTENSION, KVM_CAP_PPC_HTM);
> > > }
> > >
> > > This happens because when using the global KVM fd with
> > > KVM_CHECK_EXTENSION, kvm_vm_ioctl_check_extension() gets
> > > called with a NULL kvm argument, which gets dereferenced
> > > in is_kvmppc_hv_enabled(). Spotted while reading the code.
> > >
> > > Let's use the hv_enabled fallback variable, like everywhere
> > > else in this function.
> > >
> > > Fixes: 23528bb21ee2 ("KVM: PPC: Introduce KVM_CAP_PPC_HTM")
> > > Cc: stable@vger.kernel.org # v4.7+
> > > Signed-off-by: Greg Kurz <groug@kaod.org>
> > > ---
> > >  arch/powerpc/kvm/powerpc.c |    3 +--
> > >  1 file changed, 1 insertion(+), 2 deletions(-)
> > >
> > > diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
> > > index 3480faaf1ef8..ee279c7f4802 100644
> > > --- a/arch/powerpc/kvm/powerpc.c
> > > +++ b/arch/powerpc/kvm/powerpc.c
> > > @@ -644,8 +644,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
> > >  		break;
> > >  #endif
> > >  	case KVM_CAP_PPC_HTM:
> > > -		r = cpu_has_feature(CPU_FTR_TM_COMP) &&
> > > -		    is_kvmppc_hv_enabled(kvm);
> > > +		r = cpu_has_feature(CPU_FTR_TM_COMP) && hv_enabled;
> > >  		break;
> > >  	default:
> > >  		r = 0;  
> > 
> > Did this go anywhere?
> > 
> > cheers
> 
> I'm afraid not... and I haven't tried to ping Paul yet, since he's
> supposed to be on vacation from what I've been told.

He's back now.
Greg Kurz Oct. 12, 2017, 11:16 p.m. UTC | #8 
Ping ?

On Thu, 14 Sep 2017 23:56:25 +0200
Greg Kurz <groug@kaod.org> wrote:

> The following program causes a kernel oops:
> 
> #include <sys/types.h>
> #include <sys/stat.h>
> #include <fcntl.h>
> #include <sys/ioctl.h>
> #include <linux/kvm.h>
> 
> main()
> {
>     int fd = open("/dev/kvm", O_RDWR);
>     ioctl(fd, KVM_CHECK_EXTENSION, KVM_CAP_PPC_HTM);
> }
> 
> This happens because when using the global KVM fd with
> KVM_CHECK_EXTENSION, kvm_vm_ioctl_check_extension() gets
> called with a NULL kvm argument, which gets dereferenced
> in is_kvmppc_hv_enabled(). Spotted while reading the code.
> 
> Let's use the hv_enabled fallback variable, like everywhere
> else in this function.
> 
> Fixes: 23528bb21ee2 ("KVM: PPC: Introduce KVM_CAP_PPC_HTM")
> Cc: stable@vger.kernel.org # v4.7+
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  arch/powerpc/kvm/powerpc.c |    3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
> index 3480faaf1ef8..ee279c7f4802 100644
> --- a/arch/powerpc/kvm/powerpc.c
> +++ b/arch/powerpc/kvm/powerpc.c
> @@ -644,8 +644,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
>  		break;
>  #endif
>  	case KVM_CAP_PPC_HTM:
> -		r = cpu_has_feature(CPU_FTR_TM_COMP) &&
> -		    is_kvmppc_hv_enabled(kvm);
> +		r = cpu_has_feature(CPU_FTR_TM_COMP) && hv_enabled;
>  		break;
>  	default:
>  		r = 0;
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Paolo Bonzini Oct. 13, 2017, 4:14 p.m. UTC | #9 
On 13/10/2017 01:16, Greg Kurz wrote:
> Ping ?

When is Paul back from vacation? :)

Paolo

> On Thu, 14 Sep 2017 23:56:25 +0200
> Greg Kurz <groug@kaod.org> wrote:
> 
>> The following program causes a kernel oops:
>>
>> #include <sys/types.h>
>> #include <sys/stat.h>
>> #include <fcntl.h>
>> #include <sys/ioctl.h>
>> #include <linux/kvm.h>
>>
>> main()
>> {
>>     int fd = open("/dev/kvm", O_RDWR);
>>     ioctl(fd, KVM_CHECK_EXTENSION, KVM_CAP_PPC_HTM);
>> }
>>
>> This happens because when using the global KVM fd with
>> KVM_CHECK_EXTENSION, kvm_vm_ioctl_check_extension() gets
>> called with a NULL kvm argument, which gets dereferenced
>> in is_kvmppc_hv_enabled(). Spotted while reading the code.
>>
>> Let's use the hv_enabled fallback variable, like everywhere
>> else in this function.
>>
>> Fixes: 23528bb21ee2 ("KVM: PPC: Introduce KVM_CAP_PPC_HTM")
>> Cc: stable@vger.kernel.org # v4.7+
>> Signed-off-by: Greg Kurz <groug@kaod.org>
>> ---
>>  arch/powerpc/kvm/powerpc.c |    3 +--
>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
>> index 3480faaf1ef8..ee279c7f4802 100644
>> --- a/arch/powerpc/kvm/powerpc.c
>> +++ b/arch/powerpc/kvm/powerpc.c
>> @@ -644,8 +644,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
>>  		break;
>>  #endif
>>  	case KVM_CAP_PPC_HTM:
>> -		r = cpu_has_feature(CPU_FTR_TM_COMP) &&
>> -		    is_kvmppc_hv_enabled(kvm);
>> +		r = cpu_has_feature(CPU_FTR_TM_COMP) && hv_enabled;
>>  		break;
>>  	default:
>>  		r = 0;
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Paul Mackerras Oct. 14, 2017, 1:23 a.m. UTC | #10 
On Thu, Sep 14, 2017 at 11:56:25PM +0200, Greg Kurz wrote:
> The following program causes a kernel oops:
> 
> #include <sys/types.h>
> #include <sys/stat.h>
> #include <fcntl.h>
> #include <sys/ioctl.h>
> #include <linux/kvm.h>
> 
> main()
> {
>     int fd = open("/dev/kvm", O_RDWR);
>     ioctl(fd, KVM_CHECK_EXTENSION, KVM_CAP_PPC_HTM);
> }
> 
> This happens because when using the global KVM fd with
> KVM_CHECK_EXTENSION, kvm_vm_ioctl_check_extension() gets
> called with a NULL kvm argument, which gets dereferenced
> in is_kvmppc_hv_enabled(). Spotted while reading the code.
> 
> Let's use the hv_enabled fallback variable, like everywhere
> else in this function.
> 
> Fixes: 23528bb21ee2 ("KVM: PPC: Introduce KVM_CAP_PPC_HTM")
> Cc: stable@vger.kernel.org # v4.7+
> Signed-off-by: Greg Kurz <groug@kaod.org>

Thanks, applied to my kvm-ppc-fixes branch.

Paul.
--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Paul Mackerras Oct. 14, 2017, 1:23 a.m. UTC | #11 
On Fri, Oct 13, 2017 at 06:14:00PM +0200, Paolo Bonzini wrote:
> On 13/10/2017 01:16, Greg Kurz wrote:
> > Ping ?
> 
> When is Paul back from vacation? :)

Now. :)

Paul.
--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox series

Patch

diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
index 3480faaf1ef8..ee279c7f4802 100644
--- a/arch/powerpc/kvm/powerpc.c
+++ b/arch/powerpc/kvm/powerpc.c
@@ -644,8 +644,7 @@  int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
 		break;
 #endif
 	case KVM_CAP_PPC_HTM:
-		r = cpu_has_feature(CPU_FTR_TM_COMP) &&
-		    is_kvmppc_hv_enabled(kvm);
+		r = cpu_has_feature(CPU_FTR_TM_COMP) && hv_enabled;
 		break;
 	default:
 		r = 0;