[bug report] x86/sev: Split up runtime #VC handler for correct state tracking

[bug report] x86/sev: Split up runtime #VC handler for correct state tracking

[Dan Carpenter]

Hello Joerg Roedel,

The patch be1a5408868a: "x86/sev: Split up runtime #VC handler for
correct state tracking" from Jun 18, 2021, leads to the following
static checker warning:

	arch/x86/kernel/kvm.c:153 kvm_async_pf_task_wait_schedule()
	warn: sleeping in atomic context

arch/x86/kernel/sev.c
  1416           * Handle #DB before calling into !noinstr code to avoid recursive #DB.
  1417           */
  1418          if (vc_is_db(error_code)) {
  1419                  exc_debug(regs);
  1420                  return;
  1421          }
  1422  
  1423          irq_state = irqentry_nmi_enter(regs);
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
preempt disabled inside irqentry_nmi_enter().

  1424  
  1425          instrumentation_begin();
  1426  
  1427          if (!vc_raw_handle_exception(regs, error_code)) {
                     ^^^^^^^^^^^^^^^^^^^^^^^^

These sleeping in atomic static checker warnings come with a lot of
caveats because the call tree is very long and it's easy to have false
positives.

--> vc_raw_handle_exception()
    --> vc_forward_exception()
        --> exc_page_fault()

Page faults always sleep right?

  1428                  /* Show some debug info */
  1429                  show_regs(regs);
  1430  
  1431                  /* Ask hypervisor to sev_es_terminate */
  1432                  sev_es_terminate(GHCB_SEV_ES_REASON_GENERAL_REQUEST);
  1433  
  1434                  /* If that fails and we get here - just panic */
  1435                  panic("Returned from Terminate-Request to Hypervisor\n");
  1436          }
  1437  
  1438          instrumentation_end();
  1439          irqentry_nmi_exit(regs, irq_state);
  1440  }

regards,
dan carpenter

Re: [bug report] x86/sev: Split up runtime #VC handler for correct state tracking

[Joerg Roedel]

Hi Dan,

On Wed, Aug 04, 2021 at 12:57:25PM +0300, Dan Carpenter wrote:
> These sleeping in atomic static checker warnings come with a lot of
> caveats because the call tree is very long and it's easy to have false
> positives.
> 
> --> vc_raw_handle_exception()
>     --> vc_forward_exception()
>         --> exc_page_fault()
> 
> Page faults always sleep right?

No, page faults do no always sleep, only when IO needs to be done to
fulfill the page fault request. In this case, the page-fault handler
will never sleep, because it is called with preemption disabled. The
page-fault handler can detect this and just do nothing. The #VC handler
will return for re-fault in this case.

Hope that helps,

     Joerg

Re: [bug report] x86/sev: Split up runtime #VC handler for correct state tracking

[Dan Carpenter]

On Wed, Aug 04, 2021 at 02:38:35PM +0200, Joerg Roedel wrote:
> Hi Dan,
> 
> On Wed, Aug 04, 2021 at 12:57:25PM +0300, Dan Carpenter wrote:
> > These sleeping in atomic static checker warnings come with a lot of
> > caveats because the call tree is very long and it's easy to have false
> > positives.
> > 
> > --> vc_raw_handle_exception()
> >     --> vc_forward_exception()
> >         --> exc_page_fault()
> > 
> > Page faults always sleep right?
> 
> No, page faults do no always sleep, only when IO needs to be done to
> fulfill the page fault request. In this case, the page-fault handler
> will never sleep, because it is called with preemption disabled. The
> page-fault handler can detect this and just do nothing. The #VC handler
> will return for re-fault in this case.

Hm...  Ok.  Let give you the rest of the call tree then because I'm not
seeing where it checks preempt count.

exc_page_fault() <-- called with preempt disabled
--> kvm_handle_async_pf()
    --> __kvm_handle_async_pf()
        --> kvm_async_pf_task_wait_schedule() calls schedule().

regards,
dan carpenter

Re: [bug report] x86/sev: Split up runtime #VC handler for correct state tracking

[Joerg Roedel]

On Wed, Aug 04, 2021 at 03:58:34PM +0300, Dan Carpenter wrote:
> Hm...  Ok.  Let give you the rest of the call tree then because I'm not
> seeing where it checks preempt count.

The check is in faulthandler_disabled(), it checks for in_atomic().

Regards,

	Joerg

Re: [bug report] x86/sev: Split up runtime #VC handler for correct state tracking

[Dan Carpenter]

On Wed, Aug 04, 2021 at 03:35:01PM +0200, Joerg Roedel wrote:
> On Wed, Aug 04, 2021 at 03:58:34PM +0300, Dan Carpenter wrote:
> > Hm...  Ok.  Let give you the rest of the call tree then because I'm not
> > seeing where it checks preempt count.
> 
> The check is in faulthandler_disabled(), it checks for in_atomic().
> 

That check is later after the caller tree that I pasted as already
called schedule().

regards,
dan carpenter

Re: [bug report] x86/sev: Split up runtime #VC handler for correct state tracking

[Joerg Roedel]

On Wed, Aug 04, 2021 at 03:58:34PM +0300, Dan Carpenter wrote:
> exc_page_fault() <-- called with preempt disabled
> --> kvm_handle_async_pf()
>     --> __kvm_handle_async_pf()
>         --> kvm_async_pf_task_wait_schedule() calls schedule().

This call path can not be taken in the page-fault handler when called
from the #VC handler. To take this path the host needs to inject an
async page-fault, especially setting async pf flags, without injecting a
page-fault exception on its own ... and when the #VC handler is running.
KVM is not doing that.

Okay, the hypervisor can be malicious, but otherwise this can't happen.
To mitigate a malicious hypervisor threat here it might be a solution to
not call the page-fault handler directly from the #VC handler and let it
re-fault after the #VC handler returned.

Regards,

	Joerg

Re: [bug report] x86/sev: Split up runtime #VC handler for correct state tracking

[Dan Carpenter]

On Wed, Aug 04, 2021 at 04:23:26PM +0200, Joerg Roedel wrote:
> On Wed, Aug 04, 2021 at 03:58:34PM +0300, Dan Carpenter wrote:
> > exc_page_fault() <-- called with preempt disabled
> > --> kvm_handle_async_pf()
> >     --> __kvm_handle_async_pf()
> >         --> kvm_async_pf_task_wait_schedule() calls schedule().
> 
> This call path can not be taken in the page-fault handler when called
> from the #VC handler. To take this path the host needs to inject an
> async page-fault, especially setting async pf flags, without injecting a
> page-fault exception on its own ... and when the #VC handler is running.
> KVM is not doing that.
> 
> Okay, the hypervisor can be malicious, but otherwise this can't happen.
> To mitigate a malicious hypervisor threat here it might be a solution to
> not call the page-fault handler directly from the #VC handler and let it
> re-fault after the #VC handler returned.
> 

Thanks for taking a look at this.

Also it turns out that my check wasn't taking in_atomic() into
consideration either so I've added that.

regards,
dan carpenter