KVM: nVMX: Fix size checks in vmx_set_nested_state
diff mbox series

Message ID 20190117195558.110516-1-jmattson@google.com
State New
Headers show
Series
  • KVM: nVMX: Fix size checks in vmx_set_nested_state
Related show

Commit Message

Jim Mattson Jan. 17, 2019, 7:55 p.m. UTC
The size checks in vmx_nested_state are wrong because the calculations
are made based on the size of a pointer to a struct kvm_nested_state
rather than the size of a struct kvm_nested_state.

Reported-by: Felix Wilhelm  <fwilhelm@google.com>
Signed-off-by: Jim Mattson <jmattson@google.com>
Reviewed-by: Drew Schmitt <dasch@google.com>
Reviewed-by: Marc Orr <marcorr@google.com>
Reviewed-by: Peter Shier <pshier@google.com>
---
 arch/x86/kvm/vmx/nested.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Jim Mattson Jan. 30, 2019, 7:52 p.m. UTC | #1
On Thu, Jan 17, 2019 at 11:56 AM Jim Mattson <jmattson@google.com> wrote:
>
> The size checks in vmx_nested_state are wrong because the calculations
> are made based on the size of a pointer to a struct kvm_nested_state
> rather than the size of a struct kvm_nested_state.
>
> Reported-by: Felix Wilhelm  <fwilhelm@google.com>
> Signed-off-by: Jim Mattson <jmattson@google.com>
> Reviewed-by: Drew Schmitt <dasch@google.com>
> Reviewed-by: Marc Orr <marcorr@google.com>
> Reviewed-by: Peter Shier <pshier@google.com>
> ---
>  arch/x86/kvm/vmx/nested.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index 2616bd2c7f2c..3bb49ad91d0c 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -5351,7 +5351,7 @@ static int vmx_set_nested_state(struct kvm_vcpu *vcpu,
>                 return ret;
>
>         /* Empty 'VMXON' state is permitted */
> -       if (kvm_state->size < sizeof(kvm_state) + sizeof(*vmcs12))
> +       if (kvm_state->size < sizeof(*kvm_state) + sizeof(*vmcs12))
>                 return 0;
>
>         if (kvm_state->vmx.vmcs_pa != -1ull) {
> @@ -5395,7 +5395,7 @@ static int vmx_set_nested_state(struct kvm_vcpu *vcpu,
>             vmcs12->vmcs_link_pointer != -1ull) {
>                 struct vmcs12 *shadow_vmcs12 = get_shadow_vmcs12(vcpu);
>
> -               if (kvm_state->size < sizeof(kvm_state) + 2 * sizeof(*vmcs12))
> +               if (kvm_state->size < sizeof(*kvm_state) + 2 * sizeof(*vmcs12))
>                         return -EINVAL;
>
>                 if (copy_from_user(shadow_vmcs12,
> --
> 2.20.1.97.g81188d93c3-goog

Ping.
Jim Mattson Feb. 12, 2019, 6:09 p.m. UTC | #2
On Wed, Jan 30, 2019 at 11:52 AM Jim Mattson <jmattson@google.com> wrote:
>
> On Thu, Jan 17, 2019 at 11:56 AM Jim Mattson <jmattson@google.com> wrote:
> >
> > The size checks in vmx_nested_state are wrong because the calculations
> > are made based on the size of a pointer to a struct kvm_nested_state
> > rather than the size of a struct kvm_nested_state.
> >
> > Reported-by: Felix Wilhelm  <fwilhelm@google.com>
> > Signed-off-by: Jim Mattson <jmattson@google.com>
> > Reviewed-by: Drew Schmitt <dasch@google.com>
> > Reviewed-by: Marc Orr <marcorr@google.com>
> > Reviewed-by: Peter Shier <pshier@google.com>
> > ---
> >  arch/x86/kvm/vmx/nested.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> > index 2616bd2c7f2c..3bb49ad91d0c 100644
> > --- a/arch/x86/kvm/vmx/nested.c
> > +++ b/arch/x86/kvm/vmx/nested.c
> > @@ -5351,7 +5351,7 @@ static int vmx_set_nested_state(struct kvm_vcpu *vcpu,
> >                 return ret;
> >
> >         /* Empty 'VMXON' state is permitted */
> > -       if (kvm_state->size < sizeof(kvm_state) + sizeof(*vmcs12))
> > +       if (kvm_state->size < sizeof(*kvm_state) + sizeof(*vmcs12))
> >                 return 0;
> >
> >         if (kvm_state->vmx.vmcs_pa != -1ull) {
> > @@ -5395,7 +5395,7 @@ static int vmx_set_nested_state(struct kvm_vcpu *vcpu,
> >             vmcs12->vmcs_link_pointer != -1ull) {
> >                 struct vmcs12 *shadow_vmcs12 = get_shadow_vmcs12(vcpu);
> >
> > -               if (kvm_state->size < sizeof(kvm_state) + 2 * sizeof(*vmcs12))
> > +               if (kvm_state->size < sizeof(*kvm_state) + 2 * sizeof(*vmcs12))
> >                         return -EINVAL;
> >
> >                 if (copy_from_user(shadow_vmcs12,
> > --
> > 2.20.1.97.g81188d93c3-goog
>
> Ping.

Ping again?
Krish Sadhukhan Feb. 12, 2019, 6:43 p.m. UTC | #3
On 02/12/2019 10:09 AM, Jim Mattson wrote:
> On Wed, Jan 30, 2019 at 11:52 AM Jim Mattson <jmattson@google.com> wrote:
>> On Thu, Jan 17, 2019 at 11:56 AM Jim Mattson <jmattson@google.com> wrote:
>>> The size checks in vmx_nested_state are wrong because the calculations
>>> are made based on the size of a pointer to a struct kvm_nested_state
>>> rather than the size of a struct kvm_nested_state.
>>>
>>> Reported-by: Felix Wilhelm  <fwilhelm@google.com>
>>> Signed-off-by: Jim Mattson <jmattson@google.com>
>>> Reviewed-by: Drew Schmitt <dasch@google.com>
>>> Reviewed-by: Marc Orr <marcorr@google.com>
>>> Reviewed-by: Peter Shier <pshier@google.com>
>>> ---
>>>   arch/x86/kvm/vmx/nested.c | 4 ++--
>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
>>> index 2616bd2c7f2c..3bb49ad91d0c 100644
>>> --- a/arch/x86/kvm/vmx/nested.c
>>> +++ b/arch/x86/kvm/vmx/nested.c
>>> @@ -5351,7 +5351,7 @@ static int vmx_set_nested_state(struct kvm_vcpu *vcpu,
>>>                  return ret;
>>>
>>>          /* Empty 'VMXON' state is permitted */
>>> -       if (kvm_state->size < sizeof(kvm_state) + sizeof(*vmcs12))
>>> +       if (kvm_state->size < sizeof(*kvm_state) + sizeof(*vmcs12))
>>>                  return 0;
>>>
>>>          if (kvm_state->vmx.vmcs_pa != -1ull) {
>>> @@ -5395,7 +5395,7 @@ static int vmx_set_nested_state(struct kvm_vcpu *vcpu,
>>>              vmcs12->vmcs_link_pointer != -1ull) {
>>>                  struct vmcs12 *shadow_vmcs12 = get_shadow_vmcs12(vcpu);
>>>
>>> -               if (kvm_state->size < sizeof(kvm_state) + 2 * sizeof(*vmcs12))
>>> +               if (kvm_state->size < sizeof(*kvm_state) + 2 * sizeof(*vmcs12))
>>>                          return -EINVAL;
>>>
>>>                  if (copy_from_user(shadow_vmcs12,
>>> --
>>> 2.20.1.97.g81188d93c3-goog
>> Ping.
> Ping again?
Reviewed-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
Jim Mattson March 13, 2019, 3:42 a.m. UTC | #4
On Tue, Feb 12, 2019 at 10:43 AM Krish Sadhukhan
<krish.sadhukhan@oracle.com> wrote:
>
>
>
> On 02/12/2019 10:09 AM, Jim Mattson wrote:
> > On Wed, Jan 30, 2019 at 11:52 AM Jim Mattson <jmattson@google.com> wrote:
> >> On Thu, Jan 17, 2019 at 11:56 AM Jim Mattson <jmattson@google.com> wrote:
> >>> The size checks in vmx_nested_state are wrong because the calculations
> >>> are made based on the size of a pointer to a struct kvm_nested_state
> >>> rather than the size of a struct kvm_nested_state.
> >>>
> >>> Reported-by: Felix Wilhelm  <fwilhelm@google.com>
> >>> Signed-off-by: Jim Mattson <jmattson@google.com>
> >>> Reviewed-by: Drew Schmitt <dasch@google.com>
> >>> Reviewed-by: Marc Orr <marcorr@google.com>
> >>> Reviewed-by: Peter Shier <pshier@google.com>
> >>> ---
> >>>   arch/x86/kvm/vmx/nested.c | 4 ++--
> >>>   1 file changed, 2 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> >>> index 2616bd2c7f2c..3bb49ad91d0c 100644
> >>> --- a/arch/x86/kvm/vmx/nested.c
> >>> +++ b/arch/x86/kvm/vmx/nested.c
> >>> @@ -5351,7 +5351,7 @@ static int vmx_set_nested_state(struct kvm_vcpu *vcpu,
> >>>                  return ret;
> >>>
> >>>          /* Empty 'VMXON' state is permitted */
> >>> -       if (kvm_state->size < sizeof(kvm_state) + sizeof(*vmcs12))
> >>> +       if (kvm_state->size < sizeof(*kvm_state) + sizeof(*vmcs12))
> >>>                  return 0;
> >>>
> >>>          if (kvm_state->vmx.vmcs_pa != -1ull) {
> >>> @@ -5395,7 +5395,7 @@ static int vmx_set_nested_state(struct kvm_vcpu *vcpu,
> >>>              vmcs12->vmcs_link_pointer != -1ull) {
> >>>                  struct vmcs12 *shadow_vmcs12 = get_shadow_vmcs12(vcpu);
> >>>
> >>> -               if (kvm_state->size < sizeof(kvm_state) + 2 * sizeof(*vmcs12))
> >>> +               if (kvm_state->size < sizeof(*kvm_state) + 2 * sizeof(*vmcs12))
> >>>                          return -EINVAL;
> >>>
> >>>                  if (copy_from_user(shadow_vmcs12,
> >>> --
> >>> 2.20.1.97.g81188d93c3-goog
> >> Ping.
> > Ping again?
> Reviewed-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>

Ping again, just for grins.
Jim Mattson March 22, 2019, 4:53 p.m. UTC | #5
On Tue, Mar 12, 2019 at 8:42 PM Jim Mattson <jmattson@google.com> wrote:
>
> On Tue, Feb 12, 2019 at 10:43 AM Krish Sadhukhan
> <krish.sadhukhan@oracle.com> wrote:
> >
> >
> >
> > On 02/12/2019 10:09 AM, Jim Mattson wrote:
> > > On Wed, Jan 30, 2019 at 11:52 AM Jim Mattson <jmattson@google.com> wrote:
> > >> On Thu, Jan 17, 2019 at 11:56 AM Jim Mattson <jmattson@google.com> wrote:
> > >>> The size checks in vmx_nested_state are wrong because the calculations
> > >>> are made based on the size of a pointer to a struct kvm_nested_state
> > >>> rather than the size of a struct kvm_nested_state.
> > >>>
> > >>> Reported-by: Felix Wilhelm  <fwilhelm@google.com>
> > >>> Signed-off-by: Jim Mattson <jmattson@google.com>
> > >>> Reviewed-by: Drew Schmitt <dasch@google.com>
> > >>> Reviewed-by: Marc Orr <marcorr@google.com>
> > >>> Reviewed-by: Peter Shier <pshier@google.com>
> > >>> ---
> > >>>   arch/x86/kvm/vmx/nested.c | 4 ++--
> > >>>   1 file changed, 2 insertions(+), 2 deletions(-)
> > >>>
> > >>> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> > >>> index 2616bd2c7f2c..3bb49ad91d0c 100644
> > >>> --- a/arch/x86/kvm/vmx/nested.c
> > >>> +++ b/arch/x86/kvm/vmx/nested.c
> > >>> @@ -5351,7 +5351,7 @@ static int vmx_set_nested_state(struct kvm_vcpu *vcpu,
> > >>>                  return ret;
> > >>>
> > >>>          /* Empty 'VMXON' state is permitted */
> > >>> -       if (kvm_state->size < sizeof(kvm_state) + sizeof(*vmcs12))
> > >>> +       if (kvm_state->size < sizeof(*kvm_state) + sizeof(*vmcs12))
> > >>>                  return 0;
> > >>>
> > >>>          if (kvm_state->vmx.vmcs_pa != -1ull) {
> > >>> @@ -5395,7 +5395,7 @@ static int vmx_set_nested_state(struct kvm_vcpu *vcpu,
> > >>>              vmcs12->vmcs_link_pointer != -1ull) {
> > >>>                  struct vmcs12 *shadow_vmcs12 = get_shadow_vmcs12(vcpu);
> > >>>
> > >>> -               if (kvm_state->size < sizeof(kvm_state) + 2 * sizeof(*vmcs12))
> > >>> +               if (kvm_state->size < sizeof(*kvm_state) + 2 * sizeof(*vmcs12))
> > >>>                          return -EINVAL;
> > >>>
> > >>>                  if (copy_from_user(shadow_vmcs12,
> > >>> --
> > >>> 2.20.1.97.g81188d93c3-goog
> > >> Ping.
> > > Ping again?
> > Reviewed-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
>
> Ping again, just for grins.

Ping.
Jim Mattson April 30, 2019, 9:46 p.m. UTC | #6
On Fri, Mar 22, 2019 at 9:53 AM Jim Mattson <jmattson@google.com> wrote:
>
> On Tue, Mar 12, 2019 at 8:42 PM Jim Mattson <jmattson@google.com> wrote:
> >
> > On Tue, Feb 12, 2019 at 10:43 AM Krish Sadhukhan
> > <krish.sadhukhan@oracle.com> wrote:
> > >
> > >
> > >
> > > On 02/12/2019 10:09 AM, Jim Mattson wrote:
> > > > On Wed, Jan 30, 2019 at 11:52 AM Jim Mattson <jmattson@google.com> wrote:
> > > >> On Thu, Jan 17, 2019 at 11:56 AM Jim Mattson <jmattson@google.com> wrote:
> > > >>> The size checks in vmx_nested_state are wrong because the calculations
> > > >>> are made based on the size of a pointer to a struct kvm_nested_state
> > > >>> rather than the size of a struct kvm_nested_state.
> > > >>>
> > > >>> Reported-by: Felix Wilhelm  <fwilhelm@google.com>
> > > >>> Signed-off-by: Jim Mattson <jmattson@google.com>
> > > >>> Reviewed-by: Drew Schmitt <dasch@google.com>
> > > >>> Reviewed-by: Marc Orr <marcorr@google.com>
> > > >>> Reviewed-by: Peter Shier <pshier@google.com>
> > > >>> ---
> > > >>>   arch/x86/kvm/vmx/nested.c | 4 ++--
> > > >>>   1 file changed, 2 insertions(+), 2 deletions(-)
> > > >>>
> > > >>> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> > > >>> index 2616bd2c7f2c..3bb49ad91d0c 100644
> > > >>> --- a/arch/x86/kvm/vmx/nested.c
> > > >>> +++ b/arch/x86/kvm/vmx/nested.c
> > > >>> @@ -5351,7 +5351,7 @@ static int vmx_set_nested_state(struct kvm_vcpu *vcpu,
> > > >>>                  return ret;
> > > >>>
> > > >>>          /* Empty 'VMXON' state is permitted */
> > > >>> -       if (kvm_state->size < sizeof(kvm_state) + sizeof(*vmcs12))
> > > >>> +       if (kvm_state->size < sizeof(*kvm_state) + sizeof(*vmcs12))
> > > >>>                  return 0;
> > > >>>
> > > >>>          if (kvm_state->vmx.vmcs_pa != -1ull) {
> > > >>> @@ -5395,7 +5395,7 @@ static int vmx_set_nested_state(struct kvm_vcpu *vcpu,
> > > >>>              vmcs12->vmcs_link_pointer != -1ull) {
> > > >>>                  struct vmcs12 *shadow_vmcs12 = get_shadow_vmcs12(vcpu);
> > > >>>
> > > >>> -               if (kvm_state->size < sizeof(kvm_state) + 2 * sizeof(*vmcs12))
> > > >>> +               if (kvm_state->size < sizeof(*kvm_state) + 2 * sizeof(*vmcs12))
> > > >>>                          return -EINVAL;
> > > >>>
> > > >>>                  if (copy_from_user(shadow_vmcs12,
> > > >>> --
> > > >>> 2.20.1.97.g81188d93c3-goog
> > > >> Ping.
> > > > Ping again?
> > > Reviewed-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
> >
> > Ping again, just for grins.
>
> Ping.

This seems pretty straightforward. Am I missing something?
Paolo Bonzini April 30, 2019, 10:45 p.m. UTC | #7
On 30/04/19 23:46, Jim Mattson wrote:
> On Fri, Mar 22, 2019 at 9:53 AM Jim Mattson <jmattson@google.com> wrote:
>>
>> On Tue, Mar 12, 2019 at 8:42 PM Jim Mattson <jmattson@google.com> wrote:
>>>
>>> On Tue, Feb 12, 2019 at 10:43 AM Krish Sadhukhan
>>> <krish.sadhukhan@oracle.com> wrote:
>>>>
>>>>
>>>>
>>>> On 02/12/2019 10:09 AM, Jim Mattson wrote:
>>>>> On Wed, Jan 30, 2019 at 11:52 AM Jim Mattson <jmattson@google.com> wrote:
>>>>>> On Thu, Jan 17, 2019 at 11:56 AM Jim Mattson <jmattson@google.com> wrote:
>>>>>>> The size checks in vmx_nested_state are wrong because the calculations
>>>>>>> are made based on the size of a pointer to a struct kvm_nested_state
>>>>>>> rather than the size of a struct kvm_nested_state.
>>>>>>>
>>>>>>> Reported-by: Felix Wilhelm  <fwilhelm@google.com>
>>>>>>> Signed-off-by: Jim Mattson <jmattson@google.com>
>>>>>>> Reviewed-by: Drew Schmitt <dasch@google.com>
>>>>>>> Reviewed-by: Marc Orr <marcorr@google.com>
>>>>>>> Reviewed-by: Peter Shier <pshier@google.com>
>>>>>>> ---
>>>>>>>   arch/x86/kvm/vmx/nested.c | 4 ++--
>>>>>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>>>
>>>>>>> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
>>>>>>> index 2616bd2c7f2c..3bb49ad91d0c 100644
>>>>>>> --- a/arch/x86/kvm/vmx/nested.c
>>>>>>> +++ b/arch/x86/kvm/vmx/nested.c
>>>>>>> @@ -5351,7 +5351,7 @@ static int vmx_set_nested_state(struct kvm_vcpu *vcpu,
>>>>>>>                  return ret;
>>>>>>>
>>>>>>>          /* Empty 'VMXON' state is permitted */
>>>>>>> -       if (kvm_state->size < sizeof(kvm_state) + sizeof(*vmcs12))
>>>>>>> +       if (kvm_state->size < sizeof(*kvm_state) + sizeof(*vmcs12))
>>>>>>>                  return 0;
>>>>>>>
>>>>>>>          if (kvm_state->vmx.vmcs_pa != -1ull) {
>>>>>>> @@ -5395,7 +5395,7 @@ static int vmx_set_nested_state(struct kvm_vcpu *vcpu,
>>>>>>>              vmcs12->vmcs_link_pointer != -1ull) {
>>>>>>>                  struct vmcs12 *shadow_vmcs12 = get_shadow_vmcs12(vcpu);
>>>>>>>
>>>>>>> -               if (kvm_state->size < sizeof(kvm_state) + 2 * sizeof(*vmcs12))
>>>>>>> +               if (kvm_state->size < sizeof(*kvm_state) + 2 * sizeof(*vmcs12))
>>>>>>>                          return -EINVAL;
>>>>>>>
>>>>>>>                  if (copy_from_user(shadow_vmcs12,
>>>>>>> --
>>>>>>> 2.20.1.97.g81188d93c3-goog
>>>>>> Ping.
>>>>> Ping again?
>>>> Reviewed-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
>>>
>>> Ping again, just for grins.
>>
>> Ping.
> 
> This seems pretty straightforward. Am I missing something?

Nope, I just missed it again and again.  I've queued it now.

Paolo

Patch
diff mbox series

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index 2616bd2c7f2c..3bb49ad91d0c 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -5351,7 +5351,7 @@  static int vmx_set_nested_state(struct kvm_vcpu *vcpu,
 		return ret;
 
 	/* Empty 'VMXON' state is permitted */
-	if (kvm_state->size < sizeof(kvm_state) + sizeof(*vmcs12))
+	if (kvm_state->size < sizeof(*kvm_state) + sizeof(*vmcs12))
 		return 0;
 
 	if (kvm_state->vmx.vmcs_pa != -1ull) {
@@ -5395,7 +5395,7 @@  static int vmx_set_nested_state(struct kvm_vcpu *vcpu,
 	    vmcs12->vmcs_link_pointer != -1ull) {
 		struct vmcs12 *shadow_vmcs12 = get_shadow_vmcs12(vcpu);
 
-		if (kvm_state->size < sizeof(kvm_state) + 2 * sizeof(*vmcs12))
+		if (kvm_state->size < sizeof(*kvm_state) + 2 * sizeof(*vmcs12))
 			return -EINVAL;
 
 		if (copy_from_user(shadow_vmcs12,