[PATCH v2] x86: kvm: fix information leak to userland

[PATCH v2] x86: kvm: fix information leak to userland

[Vasiliy Kulikov]

Structure kvm_ppc_pvinfo is copied to userland with pad field
unitialized.  Structure kvm_clock_data is copied to userland with
flags and pad fields unitialized.  It leads to leaking of contents
of kernel stack memory.

Signed-off-by: Vasiliy Kulikov <segooon@gmail.com>
---
 I cannot compile this driver, so it is not tested at all.

 As it is not compilable, I've missed and typed wrong var name in v1, sorry.

 arch/x86/kvm/x86.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index b0818f6..261f3d0 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -2896,6 +2896,7 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
 	case KVM_GET_DEBUGREGS: {
 		struct kvm_debugregs dbgregs;
 
+		memset(&dbgregs, 0, sizeof(dbgregs));
 		kvm_vcpu_ioctl_x86_get_debugregs(vcpu, &dbgregs);
 
 		r = -EFAULT;
@@ -3481,11 +3482,11 @@ long kvm_arch_vm_ioctl(struct file *filp,
 		struct kvm_clock_data user_ns;
 		u64 now_ns;
 
+		memset(&user_ns, 0, sizeof(user_ns));
 		local_irq_disable();
 		now_ns = get_kernel_ns();
 		user_ns.clock = kvm->arch.kvmclock_offset + now_ns;
 		local_irq_enable();
-		user_ns.flags = 0;
 
 		r = -EFAULT;
 		if (copy_to_user(argp, &user_ns, sizeof(user_ns)))
-- 
1.7.0.4

Re: [PATCH v2] x86: kvm: fix information leak to userland

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1919 bytes --]

Am 30.10.2010 16:11, Vasiliy Kulikov wrote:
> Structure kvm_ppc_pvinfo is copied to userland with pad field
> unitialized.  Structure kvm_clock_data is copied to userland with
> flags and pad fields unitialized.  It leads to leaking of contents
> of kernel stack memory.

This description only partially matches your patch, please fix.

> 
> Signed-off-by: Vasiliy Kulikov <segooon@gmail.com>
> ---
>  I cannot compile this driver, so it is not tested at all.

Why? It should be compilable (provided you have a x86 toolchain).

> 
>  As it is not compilable, I've missed and typed wrong var name in v1, sorry.
> 
>  arch/x86/kvm/x86.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index b0818f6..261f3d0 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -2896,6 +2896,7 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
>  	case KVM_GET_DEBUGREGS: {
>  		struct kvm_debugregs dbgregs;
>  
> +		memset(&dbgregs, 0, sizeof(dbgregs));
>  		kvm_vcpu_ioctl_x86_get_debugregs(vcpu, &dbgregs);
>  
>  		r = -EFAULT;
> @@ -3481,11 +3482,11 @@ long kvm_arch_vm_ioctl(struct file *filp,
>  		struct kvm_clock_data user_ns;
>  		u64 now_ns;
>  
> +		memset(&user_ns, 0, sizeof(user_ns));
>  		local_irq_disable();
>  		now_ns = get_kernel_ns();
>  		user_ns.clock = kvm->arch.kvmclock_offset + now_ns;
>  		local_irq_enable();
> -		user_ns.flags = 0;
>  
>  		r = -EFAULT;
>  		if (copy_to_user(argp, &user_ns, sizeof(user_ns)))

I would rather clear the padding/reserved fields (in both cases). No
need to double-initialize properly set fields.

There are more interfaces in KVM for obtaining data from the kernel via
padded structures. Did you check them all (kvm_vcpu_events come to my mind)?

Nevertheless, looks like a worthwhile hardening of the KVM interfaces.

Thanks,
Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]

Re: [PATCH v2] x86: kvm: fix information leak to userland

[Vasiliy Kulikov]

On Sat, Oct 30, 2010 at 16:34 +0200, Jan Kiszka wrote:
> Am 30.10.2010 16:11, Vasiliy Kulikov wrote:
> > Structure kvm_ppc_pvinfo is copied to userland with pad field
> > unitialized.  Structure kvm_clock_data is copied to userland with
> > flags and pad fields unitialized.  It leads to leaking of contents
> > of kernel stack memory.
> 
> This description only partially matches your patch, please fix.

What do you mean?  Two structures are copied with some fields with old
stack values.  Smth valuable else?

> > 
> > Signed-off-by: Vasiliy Kulikov <segooon@gmail.com>
> > ---
> >  I cannot compile this driver, so it is not tested at all.
> 
> Why? It should be compilable (provided you have a x86 toolchain).

Hmm, I can't say...  Now it is compilable, but I precisely know that it
failed with "no such header" or smth like this.  Forget it.

> > 
> >  As it is not compilable, I've missed and typed wrong var name in v1, sorry.
> > 
> >  arch/x86/kvm/x86.c |    3 ++-
> >  1 files changed, 2 insertions(+), 1 deletions(-)
> > 
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index b0818f6..261f3d0 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -2896,6 +2896,7 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
> >  	case KVM_GET_DEBUGREGS: {
> >  		struct kvm_debugregs dbgregs;
> >  
> > +		memset(&dbgregs, 0, sizeof(dbgregs));
> >  		kvm_vcpu_ioctl_x86_get_debugregs(vcpu, &dbgregs);
> >  
> >  		r = -EFAULT;
> > @@ -3481,11 +3482,11 @@ long kvm_arch_vm_ioctl(struct file *filp,
> >  		struct kvm_clock_data user_ns;
> >  		u64 now_ns;
> >  
> > +		memset(&user_ns, 0, sizeof(user_ns));
> >  		local_irq_disable();
> >  		now_ns = get_kernel_ns();
> >  		user_ns.clock = kvm->arch.kvmclock_offset + now_ns;
> >  		local_irq_enable();
> > -		user_ns.flags = 0;
> >  
> >  		r = -EFAULT;
> >  		if (copy_to_user(argp, &user_ns, sizeof(user_ns)))
> 
> I would rather clear the padding/reserved fields (in both cases). No
> need to double-initialize properly set fields.

Maybe you're right, but from my point of view it's much safer to
explicitly set it to zeroes and then maybe change some of the fields.
Anybody like me who search for such bug will know that it is initialized
at the copy_to_user() level.  If somebody wants to add smth to this code
might move field initalizing to new function; to explore this situation
one should explore this function, etc.

E.g. I searched this bug with copy_to_user() copying struct without
explicit copy_from_user() or memset().

Weaker argument: setting fields to zeroes logically related to
copy_to_user() level, but not initialization as it is correct data unit
with some unitialized not-used-yet field.  On kernel level (without
sending it to userspace) it is OK and even faster.

> There are more interfaces in KVM for obtaining data from the kernel via
> padded structures. Did you check them all (kvm_vcpu_events come to my mind)?

Not all of them yet, at least one place with kvm_vcpu_events is not initialized too.
I'll send the patch today.


Thanks,

-- 
Vasiliy

Re: [PATCH v2] x86: kvm: fix information leak to userland

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 701 bytes --]

Am 30.10.2010 17:31, Vasiliy Kulikov wrote:
> On Sat, Oct 30, 2010 at 16:34 +0200, Jan Kiszka wrote:
>> Am 30.10.2010 16:11, Vasiliy Kulikov wrote:
>>> Structure kvm_ppc_pvinfo is copied to userland with pad field
>>> unitialized.  Structure kvm_clock_data is copied to userland with
>>> flags and pad fields unitialized.  It leads to leaking of contents
>>> of kernel stack memory.
>>
>> This description only partially matches your patch, please fix.
> 
> What do you mean?  Two structures are copied with some fields with old
> stack values.  Smth valuable else?

I mean you aren't touching ppc code in this patch, but you are fixing
more than just the kvm_clock interface.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]

[patch v2] x86: kvm: x86: fix information leak to userland

[Vasiliy Kulikov]

Structures kvm_vcpu_events, kvm_debugregs, kvm_pit_state2 and
kvm_clock_data are copied to userland with some padding and reserved
fields unitialized.  It leads to leaking of contents of kernel stack
memory.  We have to initialize them to zero.

In patch v1 Jan Kiszka suggested to fill reserved fields with zeros
instead of memset'ting the whole struct.  It makes sense as these
fields are explicitly marked as padding.  No more fields need zeroing.

Signed-off-by: Vasiliy Kulikov <segooon@gmail.com>
---
 Compile tesed only.

 arch/x86/kvm/x86.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index b0818f6..463c65b 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -2560,6 +2560,7 @@ static void kvm_vcpu_ioctl_x86_get_vcpu_events(struct kvm_vcpu *vcpu,
 		!kvm_exception_is_soft(vcpu->arch.exception.nr);
 	events->exception.nr = vcpu->arch.exception.nr;
 	events->exception.has_error_code = vcpu->arch.exception.has_error_code;
+	events->exception.pad = 0;
 	events->exception.error_code = vcpu->arch.exception.error_code;
 
 	events->interrupt.injected =
@@ -2573,12 +2574,14 @@ static void kvm_vcpu_ioctl_x86_get_vcpu_events(struct kvm_vcpu *vcpu,
 	events->nmi.injected = vcpu->arch.nmi_injected;
 	events->nmi.pending = vcpu->arch.nmi_pending;
 	events->nmi.masked = kvm_x86_ops->get_nmi_mask(vcpu);
+	events->nmi.pad = 0;
 
 	events->sipi_vector = vcpu->arch.sipi_vector;
 
 	events->flags = (KVM_VCPUEVENT_VALID_NMI_PENDING
 			 | KVM_VCPUEVENT_VALID_SIPI_VECTOR
 			 | KVM_VCPUEVENT_VALID_SHADOW);
+	memset(&events->reserved, 0, sizeof(events->reserved));
 }
 
 static int kvm_vcpu_ioctl_x86_set_vcpu_events(struct kvm_vcpu *vcpu,
@@ -2623,6 +2626,7 @@ static void kvm_vcpu_ioctl_x86_get_debugregs(struct kvm_vcpu *vcpu,
 	dbgregs->dr6 = vcpu->arch.dr6;
 	dbgregs->dr7 = vcpu->arch.dr7;
 	dbgregs->flags = 0;
+	memset(&dbgregs->reserved, 0, sizeof(dbgregs->reserved));
 }
 
 static int kvm_vcpu_ioctl_x86_set_debugregs(struct kvm_vcpu *vcpu,
@@ -3106,6 +3110,7 @@ static int kvm_vm_ioctl_get_pit2(struct kvm *kvm, struct kvm_pit_state2 *ps)
 		sizeof(ps->channels));
 	ps->flags = kvm->arch.vpit->pit_state.flags;
 	mutex_unlock(&kvm->arch.vpit->pit_state.lock);
+	memset(&ps->reserved, 0, sizeof(ps->reserved));
 	return r;
 }
 
@@ -3486,6 +3491,7 @@ long kvm_arch_vm_ioctl(struct file *filp,
 		user_ns.clock = kvm->arch.kvmclock_offset + now_ns;
 		local_irq_enable();
 		user_ns.flags = 0;
+		memset(&user_ns.pad, 0, sizeof(user_ns.pad));
 
 		r = -EFAULT;
 		if (copy_to_user(argp, &user_ns, sizeof(user_ns)))
-- 
Vasiliy

Re: [patch v2] x86: kvm: x86: fix information leak to userland

[Marcelo Tosatti]

On Sat, Oct 30, 2010 at 10:54:47PM +0400, Vasiliy Kulikov wrote:
> Structures kvm_vcpu_events, kvm_debugregs, kvm_pit_state2 and
> kvm_clock_data are copied to userland with some padding and reserved
> fields unitialized.  It leads to leaking of contents of kernel stack
> memory.  We have to initialize them to zero.
> 
> In patch v1 Jan Kiszka suggested to fill reserved fields with zeros
> instead of memset'ting the whole struct.  It makes sense as these
> fields are explicitly marked as padding.  No more fields need zeroing.
> 
> Signed-off-by: Vasiliy Kulikov <segooon@gmail.com>
> ---
>  Compile tesed only.
> 
>  arch/x86/kvm/x86.c |    6 ++++++
>  1 files changed, 6 insertions(+), 0 deletions(-)

Applied, thanks.

Re: [patch v2] x86: kvm: x86: fix information leak to userland

[Alexander Graf]

On 30.10.2010, at 20:54, Vasiliy Kulikov wrote:

> Structures kvm_vcpu_events, kvm_debugregs, kvm_pit_state2 and
> kvm_clock_data are copied to userland with some padding and reserved
> fields unitialized.  It leads to leaking of contents of kernel stack
> memory.  We have to initialize them to zero.
> 
> In patch v1 Jan Kiszka suggested to fill reserved fields with zeros
> instead of memset'ting the whole struct.  It makes sense as these
> fields are explicitly marked as padding.  No more fields need zeroing.
> 
> Signed-off-by: Vasiliy Kulikov <segooon@gmail.com>
> ---
> Compile tesed only.
> 
> arch/x86/kvm/x86.c |    6 ++++++
> 1 files changed, 6 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index b0818f6..463c65b 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -2560,6 +2560,7 @@ static void kvm_vcpu_ioctl_x86_get_vcpu_events(struct kvm_vcpu *vcpu,
> 		!kvm_exception_is_soft(vcpu->arch.exception.nr);
> 	events->exception.nr = vcpu->arch.exception.nr;
> 	events->exception.has_error_code = vcpu->arch.exception.has_error_code;
> +	events->exception.pad = 0;
> 	events->exception.error_code = vcpu->arch.exception.error_code;
> 
> 	events->interrupt.injected =
> @@ -2573,12 +2574,14 @@ static void kvm_vcpu_ioctl_x86_get_vcpu_events(struct kvm_vcpu *vcpu,
> 	events->nmi.injected = vcpu->arch.nmi_injected;
> 	events->nmi.pending = vcpu->arch.nmi_pending;
> 	events->nmi.masked = kvm_x86_ops->get_nmi_mask(vcpu);
> +	events->nmi.pad = 0;
> 
> 	events->sipi_vector = vcpu->arch.sipi_vector;
> 
> 	events->flags = (KVM_VCPUEVENT_VALID_NMI_PENDING
> 			 | KVM_VCPUEVENT_VALID_SIPI_VECTOR
> 			 | KVM_VCPUEVENT_VALID_SHADOW);
> +	memset(&events->reserved, 0, sizeof(events->reserved));
> }
> 
> static int kvm_vcpu_ioctl_x86_set_vcpu_events(struct kvm_vcpu *vcpu,
> @@ -2623,6 +2626,7 @@ static void kvm_vcpu_ioctl_x86_get_debugregs(struct kvm_vcpu *vcpu,
> 	dbgregs->dr6 = vcpu->arch.dr6;
> 	dbgregs->dr7 = vcpu->arch.dr7;
> 	dbgregs->flags = 0;
> +	memset(&dbgregs->reserved, 0, sizeof(dbgregs->reserved));
> }
> 
> static int kvm_vcpu_ioctl_x86_set_debugregs(struct kvm_vcpu *vcpu,
> @@ -3106,6 +3110,7 @@ static int kvm_vm_ioctl_get_pit2(struct kvm *kvm, struct kvm_pit_state2 *ps)
> 		sizeof(ps->channels));
> 	ps->flags = kvm->arch.vpit->pit_state.flags;
> 	mutex_unlock(&kvm->arch.vpit->pit_state.lock);
> +	memset(&ps->reserved, 0, sizeof(ps->reserved));

struct kvm_pit_state2 {
        struct kvm_pit_channel_state channels[3];
        __u32 flags;
        __u32 reserved[9];
};

So memset(&ps->reserved) would give you the a __u32 **, no? Same goes for all the other array sets in here. Or am I understanding some C logic wrong? :)


Alex

> 	return r;
> }
> 
> @@ -3486,6 +3491,7 @@ long kvm_arch_vm_ioctl(struct file *filp,
> 		user_ns.clock = kvm->arch.kvmclock_offset + now_ns;
> 		local_irq_enable();
> 		user_ns.flags = 0;
> +		memset(&user_ns.pad, 0, sizeof(user_ns.pad));
> 
> 		r = -EFAULT;
> 		if (copy_to_user(argp, &user_ns, sizeof(user_ns)))
> -- 
> Vasiliy
> --
> 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

Re: [patch v2] x86: kvm: x86: fix information leak to userland

[Avi Kivity]

On 07/26/2011 08:05 PM, Alexander Graf wrote:
> struct kvm_pit_state2 {
>          struct kvm_pit_channel_state channels[3];
>          __u32 flags;
>          __u32 reserved[9];
> };
>
> So memset(&ps->reserved) would give you the a __u32 **, no? Same goes for all the other array sets in here. Or am I understanding some C logic wrong? :)
>

An address of an array is the array itself.

-- 
error compiling committee.c: too many arguments to function

Re: [patch v2] x86: kvm: x86: fix information leak to userland

[Alexander Graf]

On 26.07.2011, at 19:24, Avi Kivity wrote:

> On 07/26/2011 08:05 PM, Alexander Graf wrote:
>> struct kvm_pit_state2 {
>>         struct kvm_pit_channel_state channels[3];
>>         __u32 flags;
>>         __u32 reserved[9];
>> };
>> 
>> So memset(&ps->reserved) would give you the a __u32 **, no? Same goes for all the other array sets in here. Or am I understanding some C logic wrong? :)
>> 
> 
> An address of an array is the array itself.

Ah, nice. So it's really a matter of taste then rather than functionality. Good to know :)


Alex

Re: [patch v2] x86: kvm: x86: fix information leak to userland

[Vasiliy Kulikov]

Alexander,

On Tue, Jul 26, 2011 at 19:05 +0200, Alexander Graf wrote:
> > @@ -2623,6 +2626,7 @@ static void kvm_vcpu_ioctl_x86_get_debugregs(struct kvm_vcpu *vcpu,
> > 	dbgregs->dr6 = vcpu->arch.dr6;
> > 	dbgregs->dr7 = vcpu->arch.dr7;
> > 	dbgregs->flags = 0;
> > +	memset(&dbgregs->reserved, 0, sizeof(dbgregs->reserved));
> > }
> > 
> > static int kvm_vcpu_ioctl_x86_set_debugregs(struct kvm_vcpu *vcpu,
> > @@ -3106,6 +3110,7 @@ static int kvm_vm_ioctl_get_pit2(struct kvm *kvm, struct kvm_pit_state2 *ps)
> > 		sizeof(ps->channels));
> > 	ps->flags = kvm->arch.vpit->pit_state.flags;
> > 	mutex_unlock(&kvm->arch.vpit->pit_state.lock);
> > +	memset(&ps->reserved, 0, sizeof(ps->reserved));
> 
> struct kvm_pit_state2 {
>         struct kvm_pit_channel_state channels[3];
>         __u32 flags;
>         __u32 reserved[9];
> };
> 
> So memset(&ps->reserved) would give you the a __u32 **, no? Same goes for all the other array sets in here. Or am I understanding some C logic wrong? :)

No, the array name and an address of the array give the same address.  I
could use ps->reserved instead of &ps->reserved, but it is a question of
coding style.  I got opposite opinions on this question from different
maintainers.

Another thing is that sizeof() of the array name and the pointer to the
first array element differ.  But I used sizeof(array) here, so it should
be correct.


Thanks,

-- 
Vasiliy Kulikov
http://www.openwall.com - bringing security into open computing environments

Re: [patch v2] x86: kvm: x86: fix information leak to userland

[Alexander Graf]

On 26.07.2011, at 19:28, Vasiliy Kulikov wrote:

> Alexander,
> 
> On Tue, Jul 26, 2011 at 19:05 +0200, Alexander Graf wrote:
>>> @@ -2623,6 +2626,7 @@ static void kvm_vcpu_ioctl_x86_get_debugregs(struct kvm_vcpu *vcpu,
>>> 	dbgregs->dr6 = vcpu->arch.dr6;
>>> 	dbgregs->dr7 = vcpu->arch.dr7;
>>> 	dbgregs->flags = 0;
>>> +	memset(&dbgregs->reserved, 0, sizeof(dbgregs->reserved));
>>> }
>>> 
>>> static int kvm_vcpu_ioctl_x86_set_debugregs(struct kvm_vcpu *vcpu,
>>> @@ -3106,6 +3110,7 @@ static int kvm_vm_ioctl_get_pit2(struct kvm *kvm, struct kvm_pit_state2 *ps)
>>> 		sizeof(ps->channels));
>>> 	ps->flags = kvm->arch.vpit->pit_state.flags;
>>> 	mutex_unlock(&kvm->arch.vpit->pit_state.lock);
>>> +	memset(&ps->reserved, 0, sizeof(ps->reserved));
>> 
>> struct kvm_pit_state2 {
>>        struct kvm_pit_channel_state channels[3];
>>        __u32 flags;
>>        __u32 reserved[9];
>> };
>> 
>> So memset(&ps->reserved) would give you the a __u32 **, no? Same goes for all the other array sets in here. Or am I understanding some C logic wrong? :)
> 
> No, the array name and an address of the array give the same address.  I
> could use ps->reserved instead of &ps->reserved, but it is a question of
> coding style.  I got opposite opinions on this question from different
> maintainers.
> 
> Another thing is that sizeof() of the array name and the pointer to the
> first array element differ.  But I used sizeof(array) here, so it should
> be correct.

Yup, the sizeof looks fine. I was really only puzzled about the &array part. But if it's standardized to return the same as array, then that's great and I can call myself more educated now :)


Alex