Re: [Xen-devel] [PATCH v6 1/6] livepatch: Disallow applying after an revert

[Ross Lagerwall]

On 09/16/2016 04:29 PM, Konrad Rzeszutek Wilk wrote:

On general this is unhealthy - as the payload's .bss (definitly)
or .data (maybe) will be modified once the payload is running.

Doing an revert and then re-applying the payload with a non-pristine
.bss or .data can lead to unforseen consequences (.bss are assumed
to always contain zero value but now they may have a different value).

There is one exception - if the payload contains only one .data section
- the .livepatch.funcs, then it is OK to re-apply an revert.

Suggested-by: Jan Beulich 
Signed-off-by: Konrad Rzeszutek Wilk 



This looks reasonable, with Jan's comments addressed.

--
Ross Lagerwall

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v6 1/6] livepatch: Disallow applying after an revert

[Jan Beulich]

>>> On 16.09.16 at 17:29,  wrote:
> On general this is unhealthy - as the payload's .bss (definitly)
> or .data (maybe) will be modified once the payload is running.
> 
> Doing an revert and then re-applying the payload with a non-pristine
> .bss or .data can lead to unforseen consequences (.bss are assumed
> to always contain zero value but now they may have a different value).
> 
> There is one exception - if the payload contains only one .data section
> - the .livepatch.funcs, then it is OK to re-apply an revert.
> 
> Suggested-by: Jan Beulich 
> Signed-off-by: Konrad Rzeszutek Wilk 
> 
> ---
> Cc: Andrew Cooper 
> Cc: Jan Beulich 

I think Ross is actually the primary person to be listed here.

> --- a/xen/common/livepatch.c
> +++ b/xen/common/livepatch.c
> @@ -52,6 +52,8 @@ struct livepatch_build_id {
>  struct payload {
>  uint32_t state;  /* One of the LIVEPATCH_STATE_*. */
>  int32_t rc;  /* 0 or -XEN_EXX. */
> +bool_t reverted; /* Whether it was reverted. */
> +bool_t safe_to_reapply;  /* Can apply safely after revert. */

You nicely use "true" int the code below - please also use plain "bool"
here.

> @@ -381,7 +383,11 @@ static int move_payload(struct payload *payload, struct 
> livepatch_elf *elf)
>  if ( elf->sec[i].sec->sh_flags & SHF_EXECINSTR )
>  buf = text_buf;
>  else if ( elf->sec[i].sec->sh_flags & SHF_WRITE )
> +{
>  buf = rw_buf;
> +rw_buf_sec = i;
> +rw_buf_cnt++;

Wouldn't it make sense to exclude zero-size sections here? Perhaps
even worth skipping them together with !SHF_ALLOC ones (and then
of course also in the other earlier loop)? The more that ISTR you
mentioning that empty .data and .bss get created by the assembler
even if there's no respective source contribution?

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[Xen-devel] [PATCH v6 1/6] livepatch: Disallow applying after an revert

[Konrad Rzeszutek Wilk]

On general this is unhealthy - as the payload's .bss (definitly)
or .data (maybe) will be modified once the payload is running.

Doing an revert and then re-applying the payload with a non-pristine
.bss or .data can lead to unforseen consequences (.bss are assumed
to always contain zero value but now they may have a different value).

There is one exception - if the payload contains only one .data section
- the .livepatch.funcs, then it is OK to re-apply an revert.

Suggested-by: Jan Beulich 
Signed-off-by: Konrad Rzeszutek Wilk 

---
Cc: Andrew Cooper 
Cc: Jan Beulich 

v6: New submission
---
 docs/misc/livepatch.markdown |  7 +++
 xen/common/livepatch.c   | 27 ++-
 2 files changed, 33 insertions(+), 1 deletion(-)

diff --git a/docs/misc/livepatch.markdown b/docs/misc/livepatch.markdown
index 89c1050..81f4fc9 100644
--- a/docs/misc/livepatch.markdown
+++ b/docs/misc/livepatch.markdown
@@ -1061,6 +1061,13 @@ depending on the current state of data. As such it 
should not be attempted.
 That said we should provide hook functions so that the existing data
 can be changed during payload application.
 
+To guarantee safety we disallow re-applying an payload after it has been
+reverted. This is because we cannot guarantee that the state of .bss
+and .data to be exactly as it was during loading. Hence the administrator
+MUST unload the payload and upload it again to apply it.
+
+There is an exception to this: if the payload only has .livepatch.funcs;
+and no .data or .bss sections.
 
 ### Inline patching
 
diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index 23e4d51..967985c 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -52,6 +52,8 @@ struct livepatch_build_id {
 struct payload {
 uint32_t state;  /* One of the LIVEPATCH_STATE_*. */
 int32_t rc;  /* 0 or -XEN_EXX. */
+bool_t reverted; /* Whether it was reverted. */
+bool_t safe_to_reapply;  /* Can apply safely after revert. */
 struct list_head list;   /* Linked to 'payload_list'. */
 const void *text_addr;   /* Virtual address of .text. */
 size_t text_size;/* .. and its size. */
@@ -308,7 +310,7 @@ static void calc_section(const struct livepatch_elf_sec 
*sec, size_t *size,
 static int move_payload(struct payload *payload, struct livepatch_elf *elf)
 {
 void *text_buf, *ro_buf, *rw_buf;
-unsigned int i;
+unsigned int i, rw_buf_sec, rw_buf_cnt = 0;
 size_t size = 0;
 unsigned int *offset;
 int rc = 0;
@@ -381,7 +383,11 @@ static int move_payload(struct payload *payload, struct 
livepatch_elf *elf)
 if ( elf->sec[i].sec->sh_flags & SHF_EXECINSTR )
 buf = text_buf;
 else if ( elf->sec[i].sec->sh_flags & SHF_WRITE )
+{
 buf = rw_buf;
+rw_buf_sec = i;
+rw_buf_cnt++;
+}
 else
 buf = ro_buf;
 
@@ -402,6 +408,10 @@ static int move_payload(struct payload *payload, struct 
livepatch_elf *elf)
 }
 }
 
+/* No .bss and no .data, and only on RW - .livepatch.funcs section. */
+if ( rw_buf_cnt == 1 &&
+ !strcmp(elf->sec[rw_buf_sec].name, ELF_LIVEPATCH_FUNC) )
+payload->safe_to_reapply = true;
  out:
 xfree(offset);
 
@@ -1057,6 +1067,7 @@ static int revert_payload(struct payload *data)
 list_del_rcu(>applied_list);
 unregister_virtual_region(>region);
 
+data->reverted = true;
 return 0;
 }
 
@@ -1438,6 +1449,20 @@ static int 
livepatch_action(xen_sysctl_livepatch_action_t *action)
 case LIVEPATCH_ACTION_APPLY:
 if ( data->state == LIVEPATCH_STATE_CHECKED )
 {
+/*
+ * It is unsafe to apply an reverted payload as the .data (or .bss)
+ * may not be in in pristine condition. Hence MUST unload and then
+ * apply patch again. Unless the payload has no .bss or only one
+ * RW section (.livepatch.funcs).
+ */
+if ( data->reverted && !data->safe_to_reapply )
+{
+dprintk(XENLOG_ERR, "%s%s: can't revert as payload has .data. 
Please unload!\n",
+LIVEPATCH, data->name);
+data->rc = -EINVAL;
+break;
+}
+
 rc = build_id_dep(data, !!list_empty(_list));
 if ( rc )
 break;
-- 
2.5.5


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel