Re: [Xen-devel] [PATCH v3 08/12] livepatch: Add support for inline asm hotpatching expectations

["Wieczorkiewicz, Pawel" <wipawel@xxxxxxxxx>]

> On 19. Sep 2019, at 18:06, Ross Lagerwall <ross.lagerwall@xxxxxxxxxx> wrote:
> 
> On 9/16/19 11:59 AM, Pawel Wieczorkiewicz wrote:
>> This is the initial implementation of the expectations enhancement
>> to improve inline asm hotpatching.
>> Expectations are designed as optional feature, since the main use of
>> them is planned for inline asm hotpatching. The flag enabled allows
>> to control the expectation state.
>> Each expectation has data and len fields that describe the data
>> that is expected to be found at a given patching (old_addr) location.
>> The len must not exceed the data array size. The data array size
>> follows the size of the opaque array, since the opaque array holds
>> the original data and therefore must match what is specified in the
>> expectation (if enabled).
>> The payload structure is modified as each expectation structure is
>> part of the livepatch_func structure and hence extends the payload.
>> Each expectation is checked prior to the apply action (i.e. as late
>> as possible to check against the most current state of the code).
>> For the replace action a new payload's expectations are checked AFTER
>> all applied payloads are successfully reverted, but BEFORE new payload
>> is applied. That breaks the replace action's atomicity and in case of
>> an expectation check failure would leave a system with all payloads
>> reverted. That is obviously insecure. Use it with caution and act
>> upon replace errors!
> snip
>>   * Lookup specified section and when exists assign its address to a 
>> specified hook.
>>   * Perform section pointer and size validation: single hook sections must 
>> contain a
>> @@ -1345,6 +1400,20 @@ static void livepatch_do_action(void)
>>            if ( rc == 0 )
>>          {
>> +            /*
>> +             * Make sure all expectation requirements are met.
>> +             * Beware all the payloads are reverted at this point.
>> +             * If expectations are not met the system is left in a
>> +             * completely UNPATCHED state!
>> +             */
>> +            rc = livepatch_check_expectations(data);
>> +            if ( rc )
>> +            {
>> +                printk(XENLOG_ERR LIVEPATCH "%s: SYSTEM MIGHT BE INSECURE: "
>> +                       "Replace action has been aborted after reverting ALL 
>> payloads!\n", data->name);
>> +                break;
>> +            }
>> +
>>              if ( is_hook_enabled(data->hooks.apply.action) )
>>              {
>>                  printk(XENLOG_INFO LIVEPATCH "%s: Calling apply action hook 
>> function\n", data->name);
>> @@ -1798,6 +1867,11 @@ static int livepatch_action(struct 
>> xen_sysctl_livepatch_action *action)
>>                      break;
>>              }
>>  +            /* Make sure all expectation requirements are met. */
>> +            rc = livepatch_check_expectations(data);
>> +            if ( rc )
>> +                break;
>> +
>>              if ( is_hook_enabled(data->hooks.apply.pre) )
>>              {
>>                  printk(XENLOG_INFO LIVEPATCH "%s: Calling pre-apply hook 
>> function\n", data->name);
> 
> I wonder if this should be done in the critical region for consistency with 
> the replace code and to minimize the chance of something going wrong between 
> calling the sysctl and the patching actually happening. Thoughts?
> 

I would not do it. At least not at the moment.
The intention behind the expectation feature is to prevent an attempt to load a 
livepatch module with some inline asm patching on a machine,
whose memory content of particular .text* section does not match.

I wanted to do it as early as possible, before any mutating action of a pre 
apply hook (an for inline asm patching preapply hook can be (ab)used often) 
kicks in.
Also, it would be good to have the expectation check always there, even with 
replaced default apply/revert action hooks.

Except from the replace action situation, the memory content of the affected 
.text* section is unlikely to change, so it should be good enough for the 
general case.
When it comes to the replace action, we should discuss it further I think, and 
maybe devise a solution on top of the current one.

> The patch looks fine otherwise.
> 

Thanks!

> Ross

Best Regards,
Pawel Wieczorkiewicz






Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879




_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel