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

[Jan Beulich]

>>> Konrad Rzeszutek Wilk  09/23/16 1:56 AM >>>
>@@ -46,6 +46,10 @@ void livepatch_elf_free(struct livepatch_elf *elf);
>int livepatch_elf_resolve_symbols(struct livepatch_elf *elf);
>int livepatch_elf_perform_relocs(struct livepatch_elf *elf);
 >
>+static inline bool livepatch_elf_ignore_section(const Elf_Shdr *sec)
>+{
>+return ( !(sec->sh_flags & SHF_ALLOC) || sec->sh_size == 0 );

With the stray blanks (and perhaps even the outer parentheses) removed,
Reviewed-by: Jan Beulich 


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

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

[Konrad Rzeszutek Wilk]

On Thu, Sep 22, 2016 at 03:21:00AM -0600, Jan Beulich wrote:
> >>> On 21.09.16 at 18:57,  wrote:
> > @@ -325,8 +327,13 @@ static int move_payload(struct payload *payload, 
> > struct livepatch_elf *elf)
> >   * and .shstrtab. For the non-relocate we allocate and copy these
> >   * via other means - and the .rel we can ignore as we only use it
> >   * once during loading.
> > + *
> > + * Also ignore sections with zero size. Those can be .data, or 
> > .bss.
> 
> Or any others. Please make this apparent by adding "e.g." or some
> such.
> 
> > + *
> > + * This logic must MATCH what is done in 
> > livepatch_elf_resolve_symbols.
> 
> Instead of such a comment, is it perhaps worth making an inline
> function or macro to cover the three instances where these
> checks need to match up?
> 
> > @@ -374,14 +381,18 @@ static int move_payload(struct payload *payload, 
> > struct livepatch_elf *elf)
> >  
> >  for ( i = 1; i < elf->hdr->e_shnum; i++ )
> >  {
> > -if ( elf->sec[i].sec->sh_flags & SHF_ALLOC )
> > +if ( elf->sec[i].sec->sh_flags & SHF_ALLOC && 
> > elf->sec[i].sec->sh_size )
> 
> Please parenthesize the & in cases like this.


Thanks!

Please see the updated patch:
From 829aa410c06231906b5ee786b10ede343ac39884 Mon Sep 17 00:00:00 2001
From: Konrad Rzeszutek Wilk 
Date: Tue, 13 Sep 2016 12:02:20 -0400
Subject: [PATCH v8] livepatch: Disallow applying after an revert

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.
We detect this rather simply (if there is one RW section and its name
is .livepatch.funcs) - but the payload may have many other RW sections
that are not used at all (such as .bss or .data sections with zero
length). To not account those we also ignore sections with sh_size
being zero.

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

---
Cc: Andrew Cooper 
Cc: Jan Beulich 

v6: New submission.
v7: Use 'bool' instead of 'bool_t'
  - Ignore sh_size==0 sections as a way to make the exception check work.
  - Also add the sh_size==0 check in livepatch_elf_resolve_symbols.
  - Update comment in load move_payload to mention
"livepatch_elf_resolve_symbols"
v8: Expand the comment
  - Parenthesize the &
  - Introduce livepatch_elf_ignore_section which does the sh_size and
SHF_ALLOC check
---
 docs/misc/livepatch.markdown|  7 +++
 xen/common/livepatch.c  | 34 +++---
 xen/common/livepatch_elf.c  |  3 +--
 xen/include/xen/livepatch_elf.h |  4 
 4 files changed, 43 insertions(+), 5 deletions(-)

diff --git a/docs/misc/livepatch.markdown b/docs/misc/livepatch.markdown
index 89c1050..a674037 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 the .data or .bss sections are of zero length.
 
 ### Inline patching
 
diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index 23e4d51..912729e 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 reverted;   /* Whether it was reverted. */
+bool 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;
  

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

[Konrad Rzeszutek Wilk]

On Thu, Sep 22, 2016 at 03:21:00AM -0600, Jan Beulich wrote:
> >>> On 21.09.16 at 18:57,  wrote:
> > @@ -325,8 +327,13 @@ static int move_payload(struct payload *payload, 
> > struct livepatch_elf *elf)
> >   * and .shstrtab. For the non-relocate we allocate and copy these
> >   * via other means - and the .rel we can ignore as we only use it
> >   * once during loading.
> > + *
> > + * Also ignore sections with zero size. Those can be .data, or 
> > .bss.
> 
> Or any others. Please make this apparent by adding "e.g." or some
> such.


> 
> > + *
> > + * This logic must MATCH what is done in 
> > livepatch_elf_resolve_symbols.
> 
> Instead of such a comment, is it perhaps worth making an inline
> function or macro to cover the three instances where these
> checks need to match up?

Yes. That would be much simpler.
> 
> > @@ -374,14 +381,18 @@ static int move_payload(struct payload *payload, 
> > struct livepatch_elf *elf)
> >  
> >  for ( i = 1; i < elf->hdr->e_shnum; i++ )
> >  {
> > -if ( elf->sec[i].sec->sh_flags & SHF_ALLOC )
> > +if ( elf->sec[i].sec->sh_flags & SHF_ALLOC && 
> > elf->sec[i].sec->sh_size )
> 
> Please parenthesize the & in cases like this.
> 
> Jan
> 

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

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

[Jan Beulich]

>>> On 21.09.16 at 18:57,  wrote:
> @@ -325,8 +327,13 @@ static int move_payload(struct payload *payload, struct 
> livepatch_elf *elf)
>   * and .shstrtab. For the non-relocate we allocate and copy these
>   * via other means - and the .rel we can ignore as we only use it
>   * once during loading.
> + *
> + * Also ignore sections with zero size. Those can be .data, or .bss.

Or any others. Please make this apparent by adding "e.g." or some
such.

> + *
> + * This logic must MATCH what is done in 
> livepatch_elf_resolve_symbols.

Instead of such a comment, is it perhaps worth making an inline
function or macro to cover the three instances where these
checks need to match up?

> @@ -374,14 +381,18 @@ static int move_payload(struct payload *payload, struct 
> livepatch_elf *elf)
>  
>  for ( i = 1; i < elf->hdr->e_shnum; i++ )
>  {
> -if ( elf->sec[i].sec->sh_flags & SHF_ALLOC )
> +if ( elf->sec[i].sec->sh_flags & SHF_ALLOC && 
> elf->sec[i].sec->sh_size )

Please parenthesize the & in cases like this.

Jan


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

[Xen-devel] [PATCH v7 1/5] 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.
We detect this rather simply (if there is one RW section and its name
is .livepatch.funcs) - but the payload may have many other RW sections
that are not used at all (such as .bss or .data sections with zero
length). To not account those we also ignore sections with sh_size
being zero.

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

---
Cc: Andrew Cooper 
Cc: Jan Beulich 

v6: New submission.
v7: Use 'bool' instead of 'bool_t'
  - Ignore sh_size==0 sections as a way to make the exception check work.
  - Also add the sh_size==0 check in livepatch_elf_resolve_symbols.
  - Update comment in load move_payload to mention
"livepatch_elf_resolve_symbols"
---
 docs/misc/livepatch.markdown |  7 +++
 xen/common/livepatch.c   | 36 +---
 xen/common/livepatch_elf.c   |  3 ++-
 3 files changed, 42 insertions(+), 4 deletions(-)

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..1f527a3 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 reverted;   /* Whether it was reverted. */
+bool 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;
@@ -325,8 +327,13 @@ static int move_payload(struct payload *payload, struct 
livepatch_elf *elf)
  * and .shstrtab. For the non-relocate we allocate and copy these
  * via other means - and the .rel we can ignore as we only use it
  * once during loading.
+ *
+ * Also ignore sections with zero size. Those can be .data, or .bss.
+ *
+ * This logic must MATCH what is done in livepatch_elf_resolve_symbols.
  */
-if ( !(elf->sec[i].sec->sh_flags & SHF_ALLOC) )
+if ( !(elf->sec[i].sec->sh_flags & SHF_ALLOC) ||
+ elf->sec[i].sec->sh_size == 0 )
 offset[i] = UINT_MAX;
 else if ( (elf->sec[i].sec->sh_flags & SHF_EXECINSTR) &&
!(elf->sec[i].sec->sh_flags & SHF_WRITE) )
@@ -374,14 +381,18 @@ static int move_payload(struct payload *payload, struct 
livepatch_elf *elf)
 
 for ( i = 1; i < elf->hdr->e_shnum; i++ )
 {
-if ( elf->sec[i].sec->sh_flags & SHF_ALLOC )
+if ( elf->sec[i].sec->sh_flags & SHF_ALLOC && elf->sec[i].sec->sh_size 
)
 {
 void *buf;
 
 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 +413,10 @@ static int move_payload(struct payload *payload, struct 
livepatch_elf *elf)
 }
 }
 
+/* Only one RW section with non-zero size: .livepatch.funcs */
+if ( rw_buf_cnt == 1 &&