Re: [Xen-devel] [PATCH v6 3/6] livepatch: NOP if func->new_addr is zero.

[Ross Lagerwall]

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

The NOP functionality will NOP any of the code at
the 'old_addr' or at 'name' if the 'new_addr' is zero.
The purpose of this is to NOP out calls, such as:

 e8 <4-bytes-offset>

(5 byte insn), or on ARM a 4 byte insn for branching.

We need the EIP of where we need to the NOP, and that can
be provided via the `old_addr` or `name`.

If the `old_addr` is provided we will NOP 'new_size'
amount of bytes at that location.

The amount is up to 31 instructions if desired (which is
the size of the opaque member). If there is a need to NOP
more then: a) more 'struct livepatch_func' structures need
to be present, b) we have to implement a variable size
buffer (in the future), or c) first byte an unconditional
branch skipping the to be disabled code (of course provided
there are no branch targets in the middle).

While at it, also unify the code on x86 patching so
it is a bit simpler (instead of two seperate writes
just make it one memcpy).

And introduce a general livepatch_insn_len inline function
that would depend on platform specific instruction size
(for a unconditional branch). As such we also rename the
PATCH_INSN_SIZE to ARCH_PATCH_INSN_SIZE.

Signed-off-by: Konrad Rzeszutek Wilk 


snip

diff --git a/docs/misc/livepatch.markdown b/docs/misc/livepatch.markdown
index 81f4fc9..a8e70a8 100644
--- a/docs/misc/livepatch.markdown
+++ b/docs/misc/livepatch.markdown
@@ -320,10 +320,13 @@ The size of the structure is 64 bytes on 64-bit 
hypervisors. It will be

 * `new_addr` is the address of the function that is replacing the old
   function. The address is filled in during relocation. The value **MUST** be
-  the address of the new function in the file.
+  either the address of the new function in the file, or zero if we are NOPing 
out
+  at `old_addr` (and `new_size` **MUST** not be zero).


The preceding untouched lines "...is the address of the function that is 
replacing the old function. The address is filled in during 
relocation..." are a bit stale now. Perhaps the whole paragraph needs to 
be replaced?




 * `old_size` and `new_size` contain the sizes of the respective functions in 
bytes.
-   The value of `old_size` **MUST** not be zero.
+   The value of `old_size` **MUST** not be zero. If the value of `new_addr` is
+   zero then `new_size` determines how many instruction bytes to NOP (up to
+   opaque size modulo smallest platform instruction - 1 byte x86 and 4 bytes 
on ARM).


This line is probably also stale now: "...contain the sizes of the 
respective functions in bytes..."




 * `version` is to be one.

diff --git a/xen/arch/x86/alternative.c b/xen/arch/x86/alternative.c
index 05e3eb8..6eaa10f 100644
--- a/xen/arch/x86/alternative.c
+++ b/xen/arch/x86/alternative.c
@@ -101,7 +101,7 @@ static void __init arch_init_ideal_nops(void)
 }

 /* Use this to add nops to a buffer, then text_poke the whole buffer. */
-static void init_or_livepatch add_nops(void *insns, unsigned int len)
+void init_or_livepatch add_nops(void *insns, unsigned int len)
 {
 while ( len > 0 )
 {

snip

 void arch_livepatch_apply_jmp(struct livepatch_func *func)
 {
-int32_t val;
 uint8_t *old_ptr;
-
-BUILD_BUG_ON(PATCH_INSN_SIZE > sizeof(func->opaque));
-BUILD_BUG_ON(PATCH_INSN_SIZE != (1 + sizeof(val)));
+uint8_t insn[sizeof(func->opaque)];
+unsigned int len;

 old_ptr = func->old_addr;
-memcpy(func->opaque, old_ptr, PATCH_INSN_SIZE);
+len = livepatch_insn_len(func);
+if ( !len )
+return;
+
+memcpy(func->opaque, old_ptr, len);
+if ( func->new_addr )
+{
+int32_t val;
+
+BUILD_BUG_ON(ARCH_PATCH_INSN_SIZE != (1 + sizeof(val)));
+
+insn[0] = 0xe9;
+val = func->new_addr - func->old_addr - ARCH_PATCH_INSN_SIZE;
+
+memcpy([1], , sizeof(val));
+}
+else
+add_nops(, len);


It probably doesn't make a difference, but I'd prefer this to be 
"add_nops(insn, len);".




-*old_ptr++ = 0xe9; /* Relative jump */
-val = func->new_addr - func->old_addr - PATCH_INSN_SIZE;
-memcpy(old_ptr, , sizeof(val));
+memcpy(old_ptr, insn, len);
 }

 void arch_livepatch_revert_jmp(const struct livepatch_func *func)
 {
-memcpy(func->old_addr, func->opaque, PATCH_INSN_SIZE);
+memcpy(func->old_addr, func->opaque, livepatch_insn_len(func));
 }



--
Ross Lagerwall

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

Re: [Xen-devel] [PATCH v6 3/6] livepatch: NOP if func->new_addr is zero.

[Jan Beulich]

>>> On 19.09.16 at 19:02,  wrote:
> On Mon, Sep 19, 2016 at 10:31:23AM -0600, Jan Beulich wrote:
>> >>> On 19.09.16 at 18:11,  wrote:
>> > On Mon, Sep 19, 2016 at 02:59:32AM -0600, Jan Beulich wrote:
>> >> >>> On 16.09.16 at 17:29,  wrote:
>> >> > @@ -31,11 +30,11 @@ void arch_livepatch_revive(void)
>> >> >  
>> >> >  int arch_livepatch_verify_func(const struct livepatch_func *func)
>> >> >  {
>> >> > -/* No NOP patching yet. */
>> >> > -if ( !func->new_size )
>> >> > +/* If NOPing only do up to maximum amount we can put in the 
>> >> > ->opaque. 
> */
>> >> > +if ( !func->new_addr && func->new_size > sizeof(func->opaque) )
>> >> >  return -EOPNOTSUPP;
>> >> >  
>> >> > -if ( func->old_size < PATCH_INSN_SIZE )
>> >> > +if ( func->old_size < ARCH_PATCH_INSN_SIZE )
>> >> >  return -EINVAL;
>> >> 
>> >> Is that indeed a requirement when NOPing? You can easily NOP out
>> >> just a single byte on x86. Or shouldn't in that case old_size == new_size
>> >> anyway? In which case the comment further down stating that new_size
>> > 
>> > The original intent behind .old_size was to guard against patching
>> > functions that were less than our relative jump. 
>> > 
>> > (The tools end up computing the .old_size as the size of the whole function
>> > which is fine).
>> > 
>> > But with this NOPing support, you are right - we could have now an
>> > function that is say 4 bytes long and we only need to NOP three bytes
>> > out of it (the last instruction I assume would be 'ret').
>> > 
>> > So perhaps this check needs just needs an 'else if' , like so:
>> > 
>> > int arch_livepatch_verify_func(const struct livepatch_func *func)
>> > {
>> > /* If NOPing.. */
>> > if ( !func->new_addr )
>> > {
>> > /* Only do up to maximum amount we can put in the ->opaque. */
>> > if ( func->new_size > sizeof(func->opaque) )
>> > return -EOPNOTSUPP;
>> > 
>> > /* One instruction for 'ret' and the other to NOP. */
>> > if ( func->old_size < 2 )
>> > return -EINVAL;
>> > }
>> > else if ( func->old_size < ARCH_PATCH_INSN_SIZE )
>> > return -EINVAL;
>> > 
>> > return 0;
>> > }
>> 
>> Except that I wouldn't use 2, to not exclude patching out some
>> single byte in the middle of a function, without regard to what the
>> function's actual size is.
> 
> Uh-uh.
> 
> The _new_size_ determines how many bytes to NOP (in the context of
> this patch). The old_size (where we check to be at min 2) is a safety
> valve to make sure we don't NOP something outside the function.

Well, all this looks a little fishy to me: I don't see the relation to
functions at all here. Patching can be done anywhere - at the start
of a function, in its middle, at the end, or - in extreme cases - even
spanning function boundaries.

So perhaps all you really want then (without altering that basic
concept) is new_size <= old_size?

>> >> NOP addition here, perhaps worth dropping the _jmp from the
>> >> function name (and its revert counterpart)?
>> > 
>> > Ooh, good idea. But I think it maybe better as a seperate patch (as it
>> > also touches the ARM code).
>> 
>> That's in the other series, isn't it?
> 
> It expands the existing ones. Right now in 'staging' branch we have an
> arch/arm/livepatch.c which has these functions in it.
> 
> Granted nothing compiles them, so I could break it in this patch.
> 
> But I already cobbled up the patch so may as well use it?

Oh, sure.

Jan


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

Re: [Xen-devel] [PATCH v6 3/6] livepatch: NOP if func->new_addr is zero.

[Konrad Rzeszutek Wilk]

> > > Ooh, good idea. But I think it maybe better as a seperate patch (as it
> > > also touches the ARM code).
> > 
> > That's in the other series, isn't it?
> 
> It expands the existing ones. Right now in 'staging' branch we have an
> arch/arm/livepatch.c which has these functions in it.
> 
> Granted nothing compiles them, so I could break it in this patch.
> 
> But I already cobbled up the patch so may as well use it?
> 

Tested version:

From 0b0ee8f270219f5c9092960ed8560d8e039332a9 Mon Sep 17 00:00:00 2001
From: Konrad Rzeszutek Wilk 
Date: Mon, 19 Sep 2016 12:20:27 -0400
Subject: [PATCH] livepatch: Drop _jmp from arch_livepatch_[apply,revert]_jmp

With "livepatch: NOP if func->new_addr is zero." that name
makes no more sense.

Suggested-by: Jan Beulich 
Signed-off-by: Konrad Rzeszutek Wilk 
---
v7: New submission
---
 xen/arch/arm/livepatch.c| 4 ++--
 xen/arch/x86/livepatch.c| 4 ++--
 xen/common/livepatch.c  | 4 ++--
 xen/include/xen/livepatch.h | 4 ++--
 4 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/xen/arch/arm/livepatch.c b/xen/arch/arm/livepatch.c
index 755f596..7f067a0 100644
--- a/xen/arch/arm/livepatch.c
+++ b/xen/arch/arm/livepatch.c
@@ -21,11 +21,11 @@ int arch_livepatch_verify_func(const struct livepatch_func 
*func)
 return -ENOSYS;
 }
 
-void arch_livepatch_apply_jmp(struct livepatch_func *func)
+void arch_livepatch_apply(struct livepatch_func *func)
 {
 }
 
-void arch_livepatch_revert_jmp(const struct livepatch_func *func)
+void arch_livepatch_revert(const struct livepatch_func *func)
 {
 }
 
diff --git a/xen/arch/x86/livepatch.c b/xen/arch/x86/livepatch.c
index 118770e..36bbc5f 100644
--- a/xen/arch/x86/livepatch.c
+++ b/xen/arch/x86/livepatch.c
@@ -47,7 +47,7 @@ int arch_livepatch_verify_func(const struct livepatch_func 
*func)
 return 0;
 }
 
-void arch_livepatch_apply_jmp(struct livepatch_func *func)
+void arch_livepatch_apply(struct livepatch_func *func)
 {
 uint8_t *old_ptr;
 uint8_t insn[sizeof(func->opaque)];
@@ -76,7 +76,7 @@ void arch_livepatch_apply_jmp(struct livepatch_func *func)
 memcpy(old_ptr, insn, len);
 }
 
-void arch_livepatch_revert_jmp(const struct livepatch_func *func)
+void arch_livepatch_revert(const struct livepatch_func *func)
 {
 memcpy(func->old_addr, func->opaque, livepatch_insn_len(func));
 }
diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index ed41f39..9d2e86d 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -1033,7 +1033,7 @@ static int apply_payload(struct payload *data)
 }
 
 for ( i = 0; i < data->nfuncs; i++ )
-arch_livepatch_apply_jmp(>funcs[i]);
+arch_livepatch_apply(>funcs[i]);
 
 arch_livepatch_revive();
 
@@ -1062,7 +1062,7 @@ static int revert_payload(struct payload *data)
 }
 
 for ( i = 0; i < data->nfuncs; i++ )
-arch_livepatch_revert_jmp(>funcs[i]);
+arch_livepatch_revert(>funcs[i]);
 
 arch_livepatch_revive();
 
diff --git a/xen/include/xen/livepatch.h b/xen/include/xen/livepatch.h
index 174af06..b7f66d4 100644
--- a/xen/include/xen/livepatch.h
+++ b/xen/include/xen/livepatch.h
@@ -86,8 +86,8 @@ unsigned int livepatch_insn_len(const struct livepatch_func 
*func)
 int arch_livepatch_quiesce(void);
 void arch_livepatch_revive(void);
 
-void arch_livepatch_apply_jmp(struct livepatch_func *func);
-void arch_livepatch_revert_jmp(const struct livepatch_func *func);
+void arch_livepatch_apply(struct livepatch_func *func);
+void arch_livepatch_revert(const struct livepatch_func *func);
 void arch_livepatch_post_action(void);
 
 void arch_livepatch_mask(void);
-- 
2.4.11


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

Re: [Xen-devel] [PATCH v6 3/6] livepatch: NOP if func->new_addr is zero.

[Konrad Rzeszutek Wilk]

On Mon, Sep 19, 2016 at 10:31:23AM -0600, Jan Beulich wrote:
> >>> On 19.09.16 at 18:11,  wrote:
> > On Mon, Sep 19, 2016 at 02:59:32AM -0600, Jan Beulich wrote:
> >> >>> On 16.09.16 at 17:29,  wrote:
> >> > @@ -31,11 +30,11 @@ void arch_livepatch_revive(void)
> >> >  
> >> >  int arch_livepatch_verify_func(const struct livepatch_func *func)
> >> >  {
> >> > -/* No NOP patching yet. */
> >> > -if ( !func->new_size )
> >> > +/* If NOPing only do up to maximum amount we can put in the 
> >> > ->opaque. */
> >> > +if ( !func->new_addr && func->new_size > sizeof(func->opaque) )
> >> >  return -EOPNOTSUPP;
> >> >  
> >> > -if ( func->old_size < PATCH_INSN_SIZE )
> >> > +if ( func->old_size < ARCH_PATCH_INSN_SIZE )
> >> >  return -EINVAL;
> >> 
> >> Is that indeed a requirement when NOPing? You can easily NOP out
> >> just a single byte on x86. Or shouldn't in that case old_size == new_size
> >> anyway? In which case the comment further down stating that new_size
> > 
> > The original intent behind .old_size was to guard against patching
> > functions that were less than our relative jump. 
> > 
> > (The tools end up computing the .old_size as the size of the whole function
> > which is fine).
> > 
> > But with this NOPing support, you are right - we could have now an
> > function that is say 4 bytes long and we only need to NOP three bytes
> > out of it (the last instruction I assume would be 'ret').
> > 
> > So perhaps this check needs just needs an 'else if' , like so:
> > 
> > int arch_livepatch_verify_func(const struct livepatch_func *func)
> > {
> > /* If NOPing.. */
> > if ( !func->new_addr )
> > {
> > /* Only do up to maximum amount we can put in the ->opaque. */
> > if ( func->new_size > sizeof(func->opaque) )
> > return -EOPNOTSUPP;
> > 
> > /* One instruction for 'ret' and the other to NOP. */
> > if ( func->old_size < 2 )
> > return -EINVAL;
> > }
> > else if ( func->old_size < ARCH_PATCH_INSN_SIZE )
> > return -EINVAL;
> > 
> > return 0;
> > }
> 
> Except that I wouldn't use 2, to not exclude patching out some
> single byte in the middle of a function, without regard to what the
> function's actual size is.

Uh-uh.

The _new_size_ determines how many bytes to NOP (in the context of
this patch). The old_size (where we check to be at min 2) is a safety
valve to make sure we don't NOP something outside the function.

..snip..
> >> NOP addition here, perhaps worth dropping the _jmp from the
> >> function name (and its revert counterpart)?
> > 
> > Ooh, good idea. But I think it maybe better as a seperate patch (as it
> > also touches the ARM code).
> 
> That's in the other series, isn't it?

It expands the existing ones. Right now in 'staging' branch we have an
arch/arm/livepatch.c which has these functions in it.

Granted nothing compiles them, so I could break it in this patch.

But I already cobbled up the patch so may as well use it?

From 45abdd6a0c3a6a2ca7180c7340032ac5b2b186a4 Mon Sep 17 00:00:00 2001
From: Konrad Rzeszutek Wilk 
Date: Mon, 19 Sep 2016 12:20:27 -0400
Subject: [PATCH] livepatch: Drop _jmp from arch_livepatch_[apply,revert]_jmp

With "livepatch: NOP if func->new_addr is zero." that name
makes no more sense.

Suggested-by: Jan Beulich 
Signed-off-by: Konrad Rzeszutek Wilk 
---
v7: New submission
---
 xen/arch/arm/livepatch.c| 4 ++--
 xen/arch/x86/livepatch.c| 4 ++--
 xen/include/xen/livepatch.h | 4 ++--
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/xen/arch/arm/livepatch.c b/xen/arch/arm/livepatch.c
index 755f596..7f067a0 100644
--- a/xen/arch/arm/livepatch.c
+++ b/xen/arch/arm/livepatch.c
@@ -21,11 +21,11 @@ int arch_livepatch_verify_func(const struct livepatch_func 
*func)
 return -ENOSYS;
 }
 
-void arch_livepatch_apply_jmp(struct livepatch_func *func)
+void arch_livepatch_apply(struct livepatch_func *func)
 {
 }
 
-void arch_livepatch_revert_jmp(const struct livepatch_func *func)
+void arch_livepatch_revert(const struct livepatch_func *func)
 {
 }
 
diff --git a/xen/arch/x86/livepatch.c b/xen/arch/x86/livepatch.c
index 118770e..36bbc5f 100644
--- a/xen/arch/x86/livepatch.c
+++ b/xen/arch/x86/livepatch.c
@@ -47,7 +47,7 @@ int arch_livepatch_verify_func(const struct livepatch_func 
*func)
 return 0;
 }
 
-void arch_livepatch_apply_jmp(struct livepatch_func *func)
+void arch_livepatch_apply(struct livepatch_func *func)
 {
 uint8_t *old_ptr;
 uint8_t insn[sizeof(func->opaque)];
@@ -76,7 +76,7 @@ void arch_livepatch_apply_jmp(struct livepatch_func *func)
 memcpy(old_ptr, insn, len);
 }
 
-void arch_livepatch_revert_jmp(const struct livepatch_func *func)
+void arch_livepatch_revert(const struct livepatch_func *func)
 {
 memcpy(func->old_addr, func->opaque, livepatch_insn_len(func));
 }
diff --git 

Re: [Xen-devel] [PATCH v6 3/6] livepatch: NOP if func->new_addr is zero.

[Jan Beulich]

>>> On 19.09.16 at 18:11,  wrote:
> On Mon, Sep 19, 2016 at 02:59:32AM -0600, Jan Beulich wrote:
>> >>> On 16.09.16 at 17:29,  wrote:
>> > @@ -31,11 +30,11 @@ void arch_livepatch_revive(void)
>> >  
>> >  int arch_livepatch_verify_func(const struct livepatch_func *func)
>> >  {
>> > -/* No NOP patching yet. */
>> > -if ( !func->new_size )
>> > +/* If NOPing only do up to maximum amount we can put in the ->opaque. 
>> > */
>> > +if ( !func->new_addr && func->new_size > sizeof(func->opaque) )
>> >  return -EOPNOTSUPP;
>> >  
>> > -if ( func->old_size < PATCH_INSN_SIZE )
>> > +if ( func->old_size < ARCH_PATCH_INSN_SIZE )
>> >  return -EINVAL;
>> 
>> Is that indeed a requirement when NOPing? You can easily NOP out
>> just a single byte on x86. Or shouldn't in that case old_size == new_size
>> anyway? In which case the comment further down stating that new_size
> 
> The original intent behind .old_size was to guard against patching
> functions that were less than our relative jump. 
> 
> (The tools end up computing the .old_size as the size of the whole function
> which is fine).
> 
> But with this NOPing support, you are right - we could have now an
> function that is say 4 bytes long and we only need to NOP three bytes
> out of it (the last instruction I assume would be 'ret').
> 
> So perhaps this check needs just needs an 'else if' , like so:
> 
> int arch_livepatch_verify_func(const struct livepatch_func *func)
> {
> /* If NOPing.. */
> if ( !func->new_addr )
> {
> /* Only do up to maximum amount we can put in the ->opaque. */
> if ( func->new_size > sizeof(func->opaque) )
> return -EOPNOTSUPP;
> 
> /* One instruction for 'ret' and the other to NOP. */
> if ( func->old_size < 2 )
> return -EINVAL;
> }
> else if ( func->old_size < ARCH_PATCH_INSN_SIZE )
> return -EINVAL;
> 
> return 0;
> }

Except that I wouldn't use 2, to not exclude patching out some
single byte in the middle of a function, without regard to what the
function's actual size is.

>> can be zero would also be wrong.
>> 
>> > @@ -43,23 +42,36 @@ int arch_livepatch_verify_func(const struct 
>> > livepatch_func *func)
>> >  
>> >  void arch_livepatch_apply_jmp(struct livepatch_func *func)
>> >  {
>> > -int32_t val;
>> >  uint8_t *old_ptr;
>> > -
>> > -BUILD_BUG_ON(PATCH_INSN_SIZE > sizeof(func->opaque));
>> > -BUILD_BUG_ON(PATCH_INSN_SIZE != (1 + sizeof(val)));
>> > +uint8_t insn[sizeof(func->opaque)];
>> > +unsigned int len;
>> >  
>> >  old_ptr = func->old_addr;
>> > -memcpy(func->opaque, old_ptr, PATCH_INSN_SIZE);
>> > +len = livepatch_insn_len(func);
>> > +if ( !len )
>> > +return;
>> > +
>> > +memcpy(func->opaque, old_ptr, len);
>> > +if ( func->new_addr )
>> > +{
>> > +int32_t val;
>> > +
>> > +BUILD_BUG_ON(ARCH_PATCH_INSN_SIZE != (1 + sizeof(val)));
>> > +
>> > +insn[0] = 0xe9;
>> > +val = func->new_addr - func->old_addr - ARCH_PATCH_INSN_SIZE;
>> > +
>> > +memcpy([1], , sizeof(val));
>> > +}
>> > +else
>> > +add_nops(, len);
>> >  
>> > -*old_ptr++ = 0xe9; /* Relative jump */
>> 
>> Are you btw intentionally getting rid of this comment? And with the
> 
> Not at all. Just missed it.
>> NOP addition here, perhaps worth dropping the _jmp from the
>> function name (and its revert counterpart)?
> 
> Ooh, good idea. But I think it maybe better as a seperate patch (as it
> also touches the ARM code).

That's in the other series, isn't it?

Jan


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

Re: [Xen-devel] [PATCH v6 3/6] livepatch: NOP if func->new_addr is zero.

[Konrad Rzeszutek Wilk]

On Mon, Sep 19, 2016 at 02:59:32AM -0600, Jan Beulich wrote:
> >>> On 16.09.16 at 17:29,  wrote:
> > @@ -31,11 +30,11 @@ void arch_livepatch_revive(void)
> >  
> >  int arch_livepatch_verify_func(const struct livepatch_func *func)
> >  {
> > -/* No NOP patching yet. */
> > -if ( !func->new_size )
> > +/* If NOPing only do up to maximum amount we can put in the ->opaque. 
> > */
> > +if ( !func->new_addr && func->new_size > sizeof(func->opaque) )
> >  return -EOPNOTSUPP;
> >  
> > -if ( func->old_size < PATCH_INSN_SIZE )
> > +if ( func->old_size < ARCH_PATCH_INSN_SIZE )
> >  return -EINVAL;
> 
> Is that indeed a requirement when NOPing? You can easily NOP out
> just a single byte on x86. Or shouldn't in that case old_size == new_size
> anyway? In which case the comment further down stating that new_size

The original intent behind .old_size was to guard against patching
functions that were less than our relative jump. 

(The tools end up computing the .old_size as the size of the whole function
which is fine).

But with this NOPing support, you are right - we could have now an
function that is say 4 bytes long and we only need to NOP three bytes
out of it (the last instruction I assume would be 'ret').

So perhaps this check needs just needs an 'else if' , like so:

int arch_livepatch_verify_func(const struct livepatch_func *func)
{
/* If NOPing.. */
if ( !func->new_addr )
{
/* Only do up to maximum amount we can put in the ->opaque. */
if ( func->new_size > sizeof(func->opaque) )
return -EOPNOTSUPP;

/* One instruction for 'ret' and the other to NOP. */
if ( func->old_size < 2 )
return -EINVAL;
}
else if ( func->old_size < ARCH_PATCH_INSN_SIZE )
return -EINVAL;

return 0;
}

[And update the design]
> can be zero would also be wrong.
> 
> > @@ -43,23 +42,36 @@ int arch_livepatch_verify_func(const struct 
> > livepatch_func *func)
> >  
> >  void arch_livepatch_apply_jmp(struct livepatch_func *func)
> >  {
> > -int32_t val;
> >  uint8_t *old_ptr;
> > -
> > -BUILD_BUG_ON(PATCH_INSN_SIZE > sizeof(func->opaque));
> > -BUILD_BUG_ON(PATCH_INSN_SIZE != (1 + sizeof(val)));
> > +uint8_t insn[sizeof(func->opaque)];
> > +unsigned int len;
> >  
> >  old_ptr = func->old_addr;
> > -memcpy(func->opaque, old_ptr, PATCH_INSN_SIZE);
> > +len = livepatch_insn_len(func);
> > +if ( !len )
> > +return;
> > +
> > +memcpy(func->opaque, old_ptr, len);
> > +if ( func->new_addr )
> > +{
> > +int32_t val;
> > +
> > +BUILD_BUG_ON(ARCH_PATCH_INSN_SIZE != (1 + sizeof(val)));
> > +
> > +insn[0] = 0xe9;
> > +val = func->new_addr - func->old_addr - ARCH_PATCH_INSN_SIZE;
> > +
> > +memcpy([1], , sizeof(val));
> > +}
> > +else
> > +add_nops(, len);
> >  
> > -*old_ptr++ = 0xe9; /* Relative jump */
> 
> Are you btw intentionally getting rid of this comment? And with the

Not at all. Just missed it.
> NOP addition here, perhaps worth dropping the _jmp from the
> function name (and its revert counterpart)?

Ooh, good idea. But I think it maybe better as a seperate patch (as it
also touches the ARM code).

> 
> > +static inline size_t livepatch_insn_len(const struct livepatch_func *func)
> 
> I think it would be nice to use consistent types: The current sole caller
> stores the result of the function in an unsigned int, and I see no reason
> why the function couldn't also return such.

/me nods.

> 
> Jan
> 

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

Re: [Xen-devel] [PATCH v6 3/6] livepatch: NOP if func->new_addr is zero.

[Jan Beulich]

>>> On 16.09.16 at 17:29,  wrote:
>  docs/misc/livepatch.markdown  |  7 +--
>  xen/arch/x86/alternative.c|  2 +-
>  xen/arch/x86/livepatch.c  | 40 
> +--
>  xen/common/livepatch.c|  3 ++-
>  xen/include/asm-x86/alternative.h |  1 +
>  xen/include/asm-x86/livepatch.h   | 21 
>  xen/include/xen/livepatch.h   |  9 +
>  7 files changed, 65 insertions(+), 18 deletions(-)
>  create mode 100644 xen/include/asm-x86/livepatch.h

I've noticed only while reviewing the other series that this addition
should imo be accompanied by a change to ./MAINTAINERS.

Jan


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

Re: [Xen-devel] [PATCH v6 3/6] livepatch: NOP if func->new_addr is zero.

[Jan Beulich]

>>> On 16.09.16 at 17:29,  wrote:
> @@ -31,11 +30,11 @@ void arch_livepatch_revive(void)
>  
>  int arch_livepatch_verify_func(const struct livepatch_func *func)
>  {
> -/* No NOP patching yet. */
> -if ( !func->new_size )
> +/* If NOPing only do up to maximum amount we can put in the ->opaque. */
> +if ( !func->new_addr && func->new_size > sizeof(func->opaque) )
>  return -EOPNOTSUPP;
>  
> -if ( func->old_size < PATCH_INSN_SIZE )
> +if ( func->old_size < ARCH_PATCH_INSN_SIZE )
>  return -EINVAL;

Is that indeed a requirement when NOPing? You can easily NOP out
just a single byte on x86. Or shouldn't in that case old_size == new_size
anyway? In which case the comment further down stating that new_size
can be zero would also be wrong.

> @@ -43,23 +42,36 @@ int arch_livepatch_verify_func(const struct 
> livepatch_func *func)
>  
>  void arch_livepatch_apply_jmp(struct livepatch_func *func)
>  {
> -int32_t val;
>  uint8_t *old_ptr;
> -
> -BUILD_BUG_ON(PATCH_INSN_SIZE > sizeof(func->opaque));
> -BUILD_BUG_ON(PATCH_INSN_SIZE != (1 + sizeof(val)));
> +uint8_t insn[sizeof(func->opaque)];
> +unsigned int len;
>  
>  old_ptr = func->old_addr;
> -memcpy(func->opaque, old_ptr, PATCH_INSN_SIZE);
> +len = livepatch_insn_len(func);
> +if ( !len )
> +return;
> +
> +memcpy(func->opaque, old_ptr, len);
> +if ( func->new_addr )
> +{
> +int32_t val;
> +
> +BUILD_BUG_ON(ARCH_PATCH_INSN_SIZE != (1 + sizeof(val)));
> +
> +insn[0] = 0xe9;
> +val = func->new_addr - func->old_addr - ARCH_PATCH_INSN_SIZE;
> +
> +memcpy([1], , sizeof(val));
> +}
> +else
> +add_nops(, len);
>  
> -*old_ptr++ = 0xe9; /* Relative jump */

Are you btw intentionally getting rid of this comment? And with the
NOP addition here, perhaps worth dropping the _jmp from the
function name (and its revert counterpart)?

> +static inline size_t livepatch_insn_len(const struct livepatch_func *func)

I think it would be nice to use consistent types: The current sole caller
stores the result of the function in an unsigned int, and I see no reason
why the function couldn't also return such.

Jan


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