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

[Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>]

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?
The patch looks fine otherwise.

Ross

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