| Message ID | 1472005332-32207-2-git-send-email-konrad.wilk@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 24.08.16 at 04:22, <konrad.wilk@oracle.com> wrote: > --- a/xen/common/livepatch.c > +++ b/xen/common/livepatch.c > @@ -70,6 +70,9 @@ struct payload { > unsigned int nsyms; /* Nr of entries in .strtab and symbols. */ > struct livepatch_build_id id; /* ELFNOTE_DESC(.note.gnu.build-id) of the payload. */ > struct livepatch_build_id dep; /* ELFNOTE_DESC(.livepatch.depends). */ > + void **bss; /* .bss's of the payload. */ > + size_t *bss_size; /* and their sizes. */ Is size_t wide enough in the extreme case? Perhaps yes, because I don't think we'll ever load 64-bit ELF on a 32-bit platform. > + size_t n_bss; /* Size of the array. */ As opposed to that, I think this one could be unsigned int (or else you end up with inconsistencies in {move,apply}_payload()). > @@ -374,14 +392,24 @@ static int move_payload(struct payload *payload, struct livepatch_elf *elf) > elf->name, elf->sec[i].name, elf->sec[i].load_addr); > } > else > - memset(elf->sec[i].load_addr, 0, elf->sec[i].sec->sh_size); > + { > + payload->bss[n_bss] = elf->sec[i].load_addr; > + payload->bss_size[n_bss++] = elf->sec[i].sec->sh_size; > + } > } > } > + ASSERT(n_bss == payload->n_bss); > > out: > xfree(offset); > > return rc; > + > + out_mem: > + dprintk(XENLOG_ERR, LIVEPATCH "%s: Could not allocate memory for payload!\n", > + elf->name); > + rc = -ENOMEM; > + goto out; You leak any of the three buffers here which you managed to successfully allocate. Jan
On 24/08/16 09:55, Jan Beulich wrote: >>>> On 24.08.16 at 04:22, <konrad.wilk@oracle.com> wrote: >> --- a/xen/common/livepatch.c >> +++ b/xen/common/livepatch.c >> @@ -70,6 +70,9 @@ struct payload { >> unsigned int nsyms; /* Nr of entries in .strtab and symbols. */ >> struct livepatch_build_id id; /* ELFNOTE_DESC(.note.gnu.build-id) of the payload. */ >> struct livepatch_build_id dep; /* ELFNOTE_DESC(.livepatch.depends). */ >> + void **bss; /* .bss's of the payload. */ >> + size_t *bss_size; /* and their sizes. */ > Is size_t wide enough in the extreme case? Perhaps yes, because I > don't think we'll ever load 64-bit ELF on a 32-bit platform. Even if we did, there is no chance that more than a single size_t's worth of data needs clearing, or the payload wouldn't fit in the current virtual address space. ~Andrew
On Wed, Aug 24, 2016 at 02:55:21AM -0600, Jan Beulich wrote: > >>> On 24.08.16 at 04:22, <konrad.wilk@oracle.com> wrote: > > --- a/xen/common/livepatch.c > > +++ b/xen/common/livepatch.c > > @@ -70,6 +70,9 @@ struct payload { > > unsigned int nsyms; /* Nr of entries in .strtab and symbols. */ > > struct livepatch_build_id id; /* ELFNOTE_DESC(.note.gnu.build-id) of the payload. */ > > struct livepatch_build_id dep; /* ELFNOTE_DESC(.livepatch.depends). */ > > + void **bss; /* .bss's of the payload. */ > > + size_t *bss_size; /* and their sizes. */ > > Is size_t wide enough in the extreme case? Perhaps yes, because I > don't think we'll ever load 64-bit ELF on a 32-bit platform. Nonethless having a huge .bss is a kind of extreme? Perhaps we should have an seperate patch that checks the SHT_NOBITS and disallows .bss's bigger than say 2MB? I am using 2MB as that is the limit of the livepatch binaries right now (see verify_payload: 127 if ( upload->size > MB(2) ) 128 return -EINVAL; > > > + size_t n_bss; /* Size of the array. */ > > As opposed to that, I think this one could be unsigned int (or else > you end up with inconsistencies in {move,apply}_payload()). /me nods. Changed to unsitned int. > > > @@ -374,14 +392,24 @@ static int move_payload(struct payload *payload, struct livepatch_elf *elf) > > elf->name, elf->sec[i].name, elf->sec[i].load_addr); > > } > > else > > - memset(elf->sec[i].load_addr, 0, elf->sec[i].sec->sh_size); > > + { > > + payload->bss[n_bss] = elf->sec[i].load_addr; > > + payload->bss_size[n_bss++] = elf->sec[i].sec->sh_size; > > + } > > } > > } > > + ASSERT(n_bss == payload->n_bss); > > > > out: > > xfree(offset); > > > > return rc; > > + > > + out_mem: > > + dprintk(XENLOG_ERR, LIVEPATCH "%s: Could not allocate memory for payload!\n", > > + elf->name); > > + rc = -ENOMEM; > > + goto out; > > You leak any of the three buffers here which you managed to > successfully allocate. I added a call to 'free_payload_data(payload)' there to make it a direct call to it. It is not needed per say as the caller unconditionally calls free_payload_data() if the return of any of the functions is non-zero. But in case things get moved around and that assumption goes away - having a call to free_payload_data makes sense in the function (plus it looks much more nicer to free/alloc in the same function).
>>> On 06.09.16 at 18:47, <konrad.wilk@oracle.com> wrote: > On Wed, Aug 24, 2016 at 02:55:21AM -0600, Jan Beulich wrote: >> >>> On 24.08.16 at 04:22, <konrad.wilk@oracle.com> wrote: >> > --- a/xen/common/livepatch.c >> > +++ b/xen/common/livepatch.c >> > @@ -70,6 +70,9 @@ struct payload { >> > unsigned int nsyms; /* Nr of entries in .strtab and symbols. */ >> > struct livepatch_build_id id; /* ELFNOTE_DESC(.note.gnu.build-id) of the payload. */ >> > struct livepatch_build_id dep; /* ELFNOTE_DESC(.livepatch.depends). */ >> > + void **bss; /* .bss's of the payload. */ >> > + size_t *bss_size; /* and their sizes. */ >> >> Is size_t wide enough in the extreme case? Perhaps yes, because I >> don't think we'll ever load 64-bit ELF on a 32-bit platform. > > Nonethless having a huge .bss is a kind of extreme? Perhaps we should > have an seperate patch that checks the SHT_NOBITS and disallows .bss's > bigger than say 2MB? Well, the extra check certainly wouldn't hurt, but I think before hitting the size_t limit you'd run out of address space to place the payload in (as that's iirc a less than 1Gb area). Jan
On Wed, Sep 07, 2016 at 02:02:44AM -0600, Jan Beulich wrote: > >>> On 06.09.16 at 18:47, <konrad.wilk@oracle.com> wrote: > > On Wed, Aug 24, 2016 at 02:55:21AM -0600, Jan Beulich wrote: > >> >>> On 24.08.16 at 04:22, <konrad.wilk@oracle.com> wrote: > >> > --- a/xen/common/livepatch.c > >> > +++ b/xen/common/livepatch.c > >> > @@ -70,6 +70,9 @@ struct payload { > >> > unsigned int nsyms; /* Nr of entries in .strtab and symbols. */ > >> > struct livepatch_build_id id; /* ELFNOTE_DESC(.note.gnu.build-id) of the payload. */ > >> > struct livepatch_build_id dep; /* ELFNOTE_DESC(.livepatch.depends). */ > >> > + void **bss; /* .bss's of the payload. */ > >> > + size_t *bss_size; /* and their sizes. */ > >> > >> Is size_t wide enough in the extreme case? Perhaps yes, because I > >> don't think we'll ever load 64-bit ELF on a 32-bit platform. > > > > Nonethless having a huge .bss is a kind of extreme? Perhaps we should > > have an seperate patch that checks the SHT_NOBITS and disallows .bss's > > bigger than say 2MB? > > Well, the extra check certainly wouldn't hurt, but I think before > hitting the size_t limit you'd run out of address space to place > the payload in (as that's iirc a less than 1Gb area). True. And on ARM 32|64 even smaller (2MB). Let me force an even smaller width type - say 'unsigned int'. > > Jan >
On 08/24/2016 03:22 AM, Konrad Rzeszutek Wilk wrote: > So that when we apply the patch again the .bss is cleared. > Otherwise we may find some variables containing old values. > > The payloads may contain various .bss - especially if -fdata-sections > is used which can create .bss.<name> sections. > After having thought about this again, I'm not sure it makes much sense. Any data sections in the payload are not reset to their initial values, so resetting the bss only may result in an unexpected combination of new & old data/bss. Perhaps it just needs to be documented that a payload's bss/data is untouched across revert/apply?
On Fri, Sep 09, 2016 at 02:33:18PM +0100, Ross Lagerwall wrote: > On 08/24/2016 03:22 AM, Konrad Rzeszutek Wilk wrote: > > So that when we apply the patch again the .bss is cleared. > > Otherwise we may find some variables containing old values. > > > > The payloads may contain various .bss - especially if -fdata-sections > > is used which can create .bss.<name> sections. > > > > After having thought about this again, I'm not sure it makes much sense. Any > data sections in the payload are not reset to their initial values, so > resetting the bss only may result in an unexpected combination of new & old > data/bss. Regardless of that I think clearing the .bss upon applying the livepatch is still the right thing to do. Regarding of the .data - we could have a copy of the .data the first time we load - and then during application copy over it from the original one?. > > Perhaps it just needs to be documented that a payload's bss/data is > untouched across revert/apply? It really cuts down on bugs if we clear the .bss. It is kind of ingrained in every developer that the .bss is zero-ed out at startup. > > -- > Ross Lagerwall
On 09/09/2016 02:50 PM, Konrad Rzeszutek Wilk wrote: > On Fri, Sep 09, 2016 at 02:33:18PM +0100, Ross Lagerwall wrote: >> On 08/24/2016 03:22 AM, Konrad Rzeszutek Wilk wrote: >>> So that when we apply the patch again the .bss is cleared. >>> Otherwise we may find some variables containing old values. >>> >>> The payloads may contain various .bss - especially if -fdata-sections >>> is used which can create .bss.<name> sections. >>> >> >> After having thought about this again, I'm not sure it makes much sense. Any >> data sections in the payload are not reset to their initial values, so >> resetting the bss only may result in an unexpected combination of new & old >> data/bss. > > Regardless of that I think clearing the .bss upon applying the livepatch is > still the right thing to do. > > Regarding of the .data - we could have a copy of the .data the first time > we load - and then during application copy over it from the original one?. > >> >> Perhaps it just needs to be documented that a payload's bss/data is >> untouched across revert/apply? > > It really cuts down on bugs if we clear the .bss. It is kind of ingrained in every > developer that the .bss is zero-ed out at startup. >> Sure. IMO clearing one but not resetting the other is even more unexpected. However, if we agree that it is desirable to do both, then this patch is acceptable as a step in the right direction.
>>> On 09.09.16 at 15:50, <konrad.wilk@oracle.com> wrote: > On Fri, Sep 09, 2016 at 02:33:18PM +0100, Ross Lagerwall wrote: >> On 08/24/2016 03:22 AM, Konrad Rzeszutek Wilk wrote: >> > So that when we apply the patch again the .bss is cleared. >> > Otherwise we may find some variables containing old values. >> > >> > The payloads may contain various .bss - especially if -fdata-sections >> > is used which can create .bss.<name> sections. >> > >> >> After having thought about this again, I'm not sure it makes much sense. Any >> data sections in the payload are not reset to their initial values, so >> resetting the bss only may result in an unexpected combination of new & old >> data/bss. > > Regardless of that I think clearing the .bss upon applying the livepatch is > still the right thing to do. > > Regarding of the .data - we could have a copy of the .data the first time > we load - and then during application copy over it from the original one?. > >> >> Perhaps it just needs to be documented that a payload's bss/data is >> untouched across revert/apply? > > It really cuts down on bugs if we clear the .bss. It is kind of ingrained in every > developer that the .bss is zero-ed out at startup. I don't think Ross meant to suggest to not clear it at all: Clearing it at load time is the equivalent of loading actual data into initialized sections. Jan
diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c index 5da28a3..b5aef57 100644 --- a/xen/common/livepatch.c +++ b/xen/common/livepatch.c @@ -70,6 +70,9 @@ struct payload { unsigned int nsyms; /* Nr of entries in .strtab and symbols. */ struct livepatch_build_id id; /* ELFNOTE_DESC(.note.gnu.build-id) of the payload. */ struct livepatch_build_id dep; /* ELFNOTE_DESC(.livepatch.depends). */ + void **bss; /* .bss's of the payload. */ + size_t *bss_size; /* and their sizes. */ + size_t n_bss; /* Size of the array. */ char name[XEN_LIVEPATCH_NAME_SIZE]; /* Name of it. */ }; @@ -255,12 +258,18 @@ static struct payload *find_payload(const char *name) static void free_payload_data(struct payload *payload) { /* Set to zero until "move_payload". */ - if ( !payload->pages ) - return; - - vfree((void *)payload->text_addr); + if ( payload->pages ) + { + vfree((void *)payload->text_addr); + payload->pages = 0; + } - payload->pages = 0; + if ( payload->n_bss ) + { + xfree(payload->bss); + xfree(payload->bss_size); + payload->n_bss = 0; + } } /* @@ -287,6 +296,7 @@ static int move_payload(struct payload *payload, struct livepatch_elf *elf) unsigned int i; size_t size = 0; unsigned int *offset; + unsigned int n_bss = 0; int rc = 0; offset = xmalloc_array(unsigned int, elf->hdr->e_shnum); @@ -309,7 +319,11 @@ static int move_payload(struct payload *payload, struct livepatch_elf *elf) calc_section(&elf->sec[i], &payload->text_size, &offset[i]); else if ( !(elf->sec[i].sec->sh_flags & SHF_EXECINSTR) && (elf->sec[i].sec->sh_flags & SHF_WRITE) ) + { calc_section(&elf->sec[i], &payload->rw_size, &offset[i]); + if ( elf->sec[i].sec->sh_type == SHT_NOBITS ) + n_bss++; + } else if ( !(elf->sec[i].sec->sh_flags & SHF_EXECINSTR) && !(elf->sec[i].sec->sh_flags & SHF_WRITE) ) calc_section(&elf->sec[i], &payload->ro_size, &offset[i]); @@ -334,12 +348,8 @@ static int move_payload(struct payload *payload, struct livepatch_elf *elf) size = PFN_UP(size); /* Nr of pages. */ text_buf = vmalloc_xen(size * PAGE_SIZE); if ( !text_buf ) - { - dprintk(XENLOG_ERR, LIVEPATCH "%s: Could not allocate memory for payload!\n", - elf->name); - rc = -ENOMEM; - goto out; - } + goto out_mem; + rw_buf = text_buf + PAGE_ALIGN(payload->text_size); ro_buf = rw_buf + PAGE_ALIGN(payload->rw_size); @@ -348,6 +358,14 @@ static int move_payload(struct payload *payload, struct livepatch_elf *elf) payload->rw_addr = rw_buf; payload->ro_addr = ro_buf; + payload->bss = xmalloc_array(void *, n_bss); + payload->bss_size = xmalloc_array(size_t, n_bss); + if ( !payload->bss || !payload->bss_size ) + goto out_mem; + + payload->n_bss = n_bss; + n_bss = 0; /* Reusing as counter. */ + for ( i = 1; i < elf->hdr->e_shnum; i++ ) { if ( elf->sec[i].sec->sh_flags & SHF_ALLOC ) @@ -374,14 +392,24 @@ static int move_payload(struct payload *payload, struct livepatch_elf *elf) elf->name, elf->sec[i].name, elf->sec[i].load_addr); } else - memset(elf->sec[i].load_addr, 0, elf->sec[i].sec->sh_size); + { + payload->bss[n_bss] = elf->sec[i].load_addr; + payload->bss_size[n_bss++] = elf->sec[i].sec->sh_size; + } } } + ASSERT(n_bss == payload->n_bss); out: xfree(offset); return rc; + + out_mem: + dprintk(XENLOG_ERR, LIVEPATCH "%s: Could not allocate memory for payload!\n", + elf->name); + rc = -ENOMEM; + goto out; } static int secure_payload(struct payload *payload, struct livepatch_elf *elf) @@ -997,6 +1025,10 @@ static int apply_payload(struct payload *data) printk(XENLOG_INFO LIVEPATCH "%s: Applying %u functions\n", data->name, data->nfuncs); + /* And clear the BSS for subsequent operation. */ + for ( i = 0; i < data->n_bss; i++ ) + memset(data->bss[i], 0, data->bss_size[i]); + arch_livepatch_quiesce(); for ( i = 0; i < data->nfuncs; i++ ) @@ -1513,9 +1545,9 @@ static void livepatch_printall(unsigned char key) list_for_each_entry ( data, &payload_list, list ) { - printk(" name=%s state=%s(%d) %p (.data=%p, .rodata=%p) using %u pages.\n", + printk(" name=%s state=%s(%d) %p (.data=%p, .rodata=%p) using %u pages (%zu .bss).\n", data->name, state2str(data->state), data->state, data->text_addr, - data->rw_addr, data->ro_addr, data->pages); + data->rw_addr, data->ro_addr, data->pages, data->n_bss); for ( i = 0; i < data->nfuncs; i++ ) {
So that when we apply the patch again the .bss is cleared. Otherwise we may find some variables containing old values. The payloads may contain various .bss - especially if -fdata-sections is used which can create .bss.<name> sections. Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> --- Cc: Ross Lagerwall <ross.lagerwall@citrix.com> Cc: Jan Beulich <jbeulich@suse.com> v3: Initial submission v4: s/EINVAL/EOPNOTSUPP/ Do memset in a single place Support multiple BSS sections. --- xen/common/livepatch.c | 60 ++++++++++++++++++++++++++++++++++++++------------ 1 file changed, 46 insertions(+), 14 deletions(-)