Re: [Xen-devel] [PATCH v5 1/4] livepatch/docs: Document .bss not being cleared, and .data potentially having changed values

[Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>]

On Mon, Sep 12, 2016 at 01:49:51AM -0600, Jan Beulich wrote:
> >>> On 11.09.16 at 17:48, <konrad.wilk@xxxxxxxxxx> wrote:
> > --- a/docs/misc/livepatch.markdown
> > +++ b/docs/misc/livepatch.markdown
> > @@ -875,6 +875,12 @@ section and the new function will reference the new 
> > string in the new
> >  
> >  This is implemented in the Xen Project hypervisor.
> >  
> > +Note that the .bss section is only cleared when the ELF payload is 
> > uploaded.
> > +Subsequent apply/revert/apply operation do no clear the .bss (or reset the
> > +.data to what it was when loaded). Hence it is the responsibility of the
> > +creator of the payload to reset these values to known good state if they
> > +depend on them having certain values at apply/revert states.
> 
> Was it, as an alternative, considered to disallow re-applying a
> reverted patch without re-uploading?

I was thinking about it but not exactly sure of the ramifications.

That is if you go this route - revert a patch - we could go the
next step and just unload it. But that puts some question on how
to schedule that without corruption - say we have payload A,B and we
are replacing A,B with C. A,B should be reverted and unloaded..

That means some form of list .. Ugh.

We could do a simpler way (which is what I think inline with your
suggestion). This does the trick (and needs to be split up obviously)
and also handles the case where you only have .text changes (like
for NOPs).

Interestingly, the linker always adds an .bss and .data section
even if the code does not produce it.

diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index d9eeab1..4a1abb2 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -53,6 +53,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. */
@@ -313,7 +315,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;
@@ -386,7 +388,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;
 
@@ -407,6 +413,11 @@ static int move_payload(struct payload *payload, struct 
livepatch_elf *elf)
         }
     }
 
+    if ( rw_buf_cnt == 1 )
+    {
+        if ( !strcmp(elf->sec[rw_buf_sec].name, ELF_LIVEPATCH_FUNC) )
+            payload->safe_to_reapply = true;
+    }
  out:
     xfree(offset);
 
@@ -1120,6 +1131,7 @@ static int revert_payload(struct payload *data)
     list_del_rcu(&data->applied_list);
     unregister_virtual_region(&data->region);
 
+    data->reverted = true;
     return 0;
 }
 
@@ -1501,6 +1513,19 @@ 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 as the .data may not be in
+             * in pristine condition. Hence MUST unload and then apply patch
+             * again.
+             */
+            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(&applied_list));
             if ( rc )
                 break;
diff --git a/xen/test/livepatch/Makefile b/xen/test/livepatch/Makefile
index 69b0cdd..e03a26d 100644
--- a/xen/test/livepatch/Makefile
+++ b/xen/test/livepatch/Makefile
@@ -72,7 +72,7 @@ $(LIVEPATCH): xen_hello_world_func.o xen_hello_world.o note.o
 note.o:
        $(OBJCOPY) -O binary --only-section=.note.gnu.build-id 
$(BASEDIR)/xen-syms $@.bin
        $(OBJCOPY) $(OBJCOPY_MAGIC) \
-                  --rename-section=.data=.livepatch.depends -S $@.bin $@
+                  
--rename-section=.data=.livepatch.depends,alloc,load,readonly,data,contents -S 
$@.bin $@
        rm -f $@.bin
 
 #
@@ -83,7 +83,7 @@ note.o:
 hello_world_note.o: $(LIVEPATCH)
        $(OBJCOPY) -O binary --only-section=.note.gnu.build-id $(LIVEPATCH) 
$@.bin
        $(OBJCOPY) $(OBJCOPY_MAGIC) \
-                  --rename-section=.data=.livepatch.depends -S $@.bin $@
+                  
--rename-section=.data=.livepatch.depends,,alloc,load,readonly,data,contents -S 
$@.bin $@
        rm -f $@.bin
 
 xen_bye_world.o: config.h
@@ -103,6 +103,8 @@ xen_nop.o: config.h
 .PHONY: $(LIVEPATCH_NOP)
 $(LIVEPATCH_NOP): xen_nop.o note.o
        $(LD) $(LDFLAGS) $(build_id_linker) -r -o $(LIVEPATCH_NOP) $^
+       $(OBJCOPY) --remove-section=.bss --remove-section=.data $@
+       $(OBJCOPY) --strip-unneeded $@
 
 .PHONY: livepatch
 livepatch: $(LIVEPATCH) $(LIVEPATCH_BYE) $(LIVEPATCH_REPLACE) $(LIVEPATCH_NOP)

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