Re: [Xen-devel] [PATCH for-4.9 v3 3/3] xen/livepatch: Don't crash on encountering STN_UNDEF relocations

[Julien Grall]

On 26/06/17 02:02, Konrad Rzeszutek Wilk wrote:

On Sat, Jun 24, 2017 at 06:28:16PM +0100, Julien Grall wrote:

Hi Konrad,

On 06/23/2017 03:46 PM, Konrad Rzeszutek Wilk wrote:

On Fri, Jun 23, 2017 at 03:36:51PM +0100, Julien Grall wrote:



On 23/06/17 15:35, Konrad Rzeszutek Wilk wrote:

On Fri, Jun 23, 2017 at 02:45:22PM +0100, Andrew Cooper wrote:

On 23/06/17 14:43, Julien Grall wrote:

Hi,

On 23/06/17 14:33, Andrew Cooper wrote:

On 23/06/17 14:32, Julien Grall wrote:

Hi Andrew,

I am a bit confused, the title says "PATCH for-4.9 v3 3/3". I haven't
been CCed on the first two patches. Does it mean you are only looking
at this patch to be in 4.9?


Sorry - I messed up the CC lists.  The correctness of this patch does
depend on the previous two, so all 3 are looking for inclusion.


Given that we don't have livepatch testing in osstest how much test
have we done on those 3 patches?


There is testing in OSSTest.


Hurray hurray hurray!


I've manually run each of the scenarios, including with my livepatch
which has a STN_UNDEF relocation.

I don't know what testing Konrad has done.


I run a version of the same tests that are in OSSTest (basically an earlier
version of the Perl code) and I have done it on x86 and on ARM32.

And I also run the standalone OSSTest (on x86)

And then I also do a livepatch using the livepatch-build-tools on x86 to
patch some silly function.

So from a testing perspective these patches have been tested very exhaustively.


Well it has not been tested on ARM64 :). I am about to do that.


/me facepalm.

I really need to get myself a working ARM64 box that is not expensive.


Also attached is the poor-man livepatch_test.perl script that mirrors
what OSSTest does.


I have got an error when executing the script after applying the "nop"
livepatch:

Executing: 'xen-livepatch apply xen_nop:' ..Applying xen_nop... completed
.. OK!
Executing: '[ `xl info| grep "xen_m" | grep or | sed s/.*:// | uniq | wc -l`
== 1 ]:' ..sh: 1: [: 1: unexpected operator
FAILED (got 512, expected: 0)


Yeah I only get that when I execute it from the serial console.
When I SSH it works fine.



But this looks like a script error than livepatch. Although, I was not able
to spot the error in the script.


Neither could I. I do also have an earlier variant of this testing script
in bash (with less test-cases).


I am happy with this patch series to be backported in Xen 4.9:

Release-acked-by: Julien Grall 

Can they be backported today please?

Cheers,

--
Julien Grall

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

Re: [Xen-devel] [PATCH for-4.9 v3 3/3] xen/livepatch: Don't crash on encountering STN_UNDEF relocations

[Konrad Rzeszutek Wilk]

On Sat, Jun 24, 2017 at 06:28:16PM +0100, Julien Grall wrote:
> Hi Konrad,
> 
> On 06/23/2017 03:46 PM, Konrad Rzeszutek Wilk wrote:
> > On Fri, Jun 23, 2017 at 03:36:51PM +0100, Julien Grall wrote:
> > > 
> > > 
> > > On 23/06/17 15:35, Konrad Rzeszutek Wilk wrote:
> > > > On Fri, Jun 23, 2017 at 02:45:22PM +0100, Andrew Cooper wrote:
> > > > > On 23/06/17 14:43, Julien Grall wrote:
> > > > > > Hi,
> > > > > > 
> > > > > > On 23/06/17 14:33, Andrew Cooper wrote:
> > > > > > > On 23/06/17 14:32, Julien Grall wrote:
> > > > > > > > Hi Andrew,
> > > > > > > > 
> > > > > > > > I am a bit confused, the title says "PATCH for-4.9 v3 3/3". I 
> > > > > > > > haven't
> > > > > > > > been CCed on the first two patches. Does it mean you are only 
> > > > > > > > looking
> > > > > > > > at this patch to be in 4.9?
> > > > > > > 
> > > > > > > Sorry - I messed up the CC lists.  The correctness of this patch 
> > > > > > > does
> > > > > > > depend on the previous two, so all 3 are looking for inclusion.
> > > > > > 
> > > > > > Given that we don't have livepatch testing in osstest how much test
> > > > > > have we done on those 3 patches?
> > > > > 
> > > > > There is testing in OSSTest.
> > > > 
> > > > Hurray hurray hurray!
> > > > > 
> > > > > I've manually run each of the scenarios, including with my livepatch
> > > > > which has a STN_UNDEF relocation.
> > > > > 
> > > > > I don't know what testing Konrad has done.
> > > > 
> > > > I run a version of the same tests that are in OSSTest (basically an 
> > > > earlier
> > > > version of the Perl code) and I have done it on x86 and on ARM32.
> > > > 
> > > > And I also run the standalone OSSTest (on x86)
> > > > 
> > > > And then I also do a livepatch using the livepatch-build-tools on x86 to
> > > > patch some silly function.
> > > > 
> > > > So from a testing perspective these patches have been tested very 
> > > > exhaustively.
> > > 
> > > Well it has not been tested on ARM64 :). I am about to do that.
> > 
> > /me facepalm.
> > 
> > I really need to get myself a working ARM64 box that is not expensive.
> > 
> > 
> > Also attached is the poor-man livepatch_test.perl script that mirrors
> > what OSSTest does.
> 
> I have got an error when executing the script after applying the "nop"
> livepatch:
> 
> Executing: 'xen-livepatch apply xen_nop:' ..Applying xen_nop... completed
> .. OK!
> Executing: '[ `xl info| grep "xen_m" | grep or | sed s/.*:// | uniq | wc -l`
> == 1 ]:' ..sh: 1: [: 1: unexpected operator
> FAILED (got 512, expected: 0)

Yeah I only get that when I execute it from the serial console.
When I SSH it works fine.

> 
> But this looks like a script error than livepatch. Although, I was not able
> to spot the error in the script.

Neither could I. I do also have an earlier variant of this testing script
in bash (with less test-cases).
> 
> Cheers,
> 
> -- 
> Julien Grall

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

Re: [Xen-devel] [PATCH for-4.9 v3 3/3] xen/livepatch: Don't crash on encountering STN_UNDEF relocations

[Julien Grall]

Hi Konrad,

On 06/23/2017 03:46 PM, Konrad Rzeszutek Wilk wrote:

On Fri, Jun 23, 2017 at 03:36:51PM +0100, Julien Grall wrote:



On 23/06/17 15:35, Konrad Rzeszutek Wilk wrote:

On Fri, Jun 23, 2017 at 02:45:22PM +0100, Andrew Cooper wrote:

On 23/06/17 14:43, Julien Grall wrote:

Hi,

On 23/06/17 14:33, Andrew Cooper wrote:

On 23/06/17 14:32, Julien Grall wrote:

Hi Andrew,

I am a bit confused, the title says "PATCH for-4.9 v3 3/3". I haven't
been CCed on the first two patches. Does it mean you are only looking
at this patch to be in 4.9?


Sorry - I messed up the CC lists.  The correctness of this patch does
depend on the previous two, so all 3 are looking for inclusion.


Given that we don't have livepatch testing in osstest how much test
have we done on those 3 patches?


There is testing in OSSTest.


Hurray hurray hurray!


I've manually run each of the scenarios, including with my livepatch
which has a STN_UNDEF relocation.

I don't know what testing Konrad has done.


I run a version of the same tests that are in OSSTest (basically an earlier
version of the Perl code) and I have done it on x86 and on ARM32.

And I also run the standalone OSSTest (on x86)

And then I also do a livepatch using the livepatch-build-tools on x86 to
patch some silly function.

So from a testing perspective these patches have been tested very exhaustively.


Well it has not been tested on ARM64 :). I am about to do that.


/me facepalm.

I really need to get myself a working ARM64 box that is not expensive.


Also attached is the poor-man livepatch_test.perl script that mirrors
what OSSTest does.


I have got an error when executing the script after applying the "nop" 
livepatch:


Executing: 'xen-livepatch apply xen_nop:' ..Applying xen_nop... completed
.. OK!
Executing: '[ `xl info| grep "xen_m" | grep or | sed s/.*:// | uniq | wc 
-l` == 1 ]:' ..sh: 1: [: 1: unexpected operator

FAILED (got 512, expected: 0)

But this looks like a script error than livepatch. Although, I was not 
able to spot the error in the script.


Cheers,

--
Julien Grall

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

Re: [Xen-devel] [PATCH for-4.9 v3 3/3] xen/livepatch: Don't crash on encountering STN_UNDEF relocations

[Konrad Rzeszutek Wilk]

On Fri, Jun 23, 2017 at 03:36:51PM +0100, Julien Grall wrote:
> 
> 
> On 23/06/17 15:35, Konrad Rzeszutek Wilk wrote:
> > On Fri, Jun 23, 2017 at 02:45:22PM +0100, Andrew Cooper wrote:
> > > On 23/06/17 14:43, Julien Grall wrote:
> > > > Hi,
> > > > 
> > > > On 23/06/17 14:33, Andrew Cooper wrote:
> > > > > On 23/06/17 14:32, Julien Grall wrote:
> > > > > > Hi Andrew,
> > > > > > 
> > > > > > I am a bit confused, the title says "PATCH for-4.9 v3 3/3". I 
> > > > > > haven't
> > > > > > been CCed on the first two patches. Does it mean you are only 
> > > > > > looking
> > > > > > at this patch to be in 4.9?
> > > > > 
> > > > > Sorry - I messed up the CC lists.  The correctness of this patch does
> > > > > depend on the previous two, so all 3 are looking for inclusion.
> > > > 
> > > > Given that we don't have livepatch testing in osstest how much test
> > > > have we done on those 3 patches?
> > > 
> > > There is testing in OSSTest.
> > 
> > Hurray hurray hurray!
> > > 
> > > I've manually run each of the scenarios, including with my livepatch
> > > which has a STN_UNDEF relocation.
> > > 
> > > I don't know what testing Konrad has done.
> > 
> > I run a version of the same tests that are in OSSTest (basically an earlier
> > version of the Perl code) and I have done it on x86 and on ARM32.
> > 
> > And I also run the standalone OSSTest (on x86)
> > 
> > And then I also do a livepatch using the livepatch-build-tools on x86 to
> > patch some silly function.
> > 
> > So from a testing perspective these patches have been tested very 
> > exhaustively.
> 
> Well it has not been tested on ARM64 :). I am about to do that.

/me facepalm.

I really need to get myself a working ARM64 box that is not expensive.


Also attached is the poor-man livepatch_test.perl script that mirrors
what OSSTest does.

[Do adjust the path, it is /usr/lib/debug, but it should be 
/usr/lib/debug/livepatch
with Xen 4.9]

#!/usr/bin/perl

use Data::Dumper;

my @livepatch_files = ("xen_hello_world.livepatch", 
"xen_replace_world.livepatch",
"xen_bye_world.livepatch", "xen_nop.livepatch");

my @livepatch_tests = (
{cmd => "xen-livepatch list", rc => 0},
{cmd => "xl info | grep xen_extra | grep -q \"Hello World\"", rc => 
256},
{cmd => "xen-livepatch revert xen_hello_world", rc => 256},
{cmd => "xen-livepatch load xen_hello_world.livepatch", rc => 0},
{cmd => "xen-livepatch load xen_hello_world.livepatch", rc => 256},
{cmd => "xen-livepatch list | grep -q xen_hello_world", rc => 0},
{cmd => "xl info | grep xen_extra | grep -q \"Hello World\"", rc => 0},
{cmd => "xen-livepatch revert xen_hello_world", rc => 0},
{cmd => "xl info | grep xen_extra | grep -q \"Hello World\"", rc => 
256},
{cmd => "xen-livepatch unload xen_hello_world", rc => 0},
{cmd => "xen-livepatch unload xen_hello_world", rc => 256},
{cmd => "xl info | grep xen_extra | grep -q \"Hello World\"", rc => 
256},
{cmd => "xen-livepatch load xen_hello_world.livepatch", rc => 0},
{cmd => "xen-livepatch load xen_bye_world.livepatch", rc => 0},
{cmd => "xl info | grep xen_extra | grep -q \"Bye World\"", rc => 0},
{cmd => "xen-livepatch upload xen_replace xen_replace_world.livepatch", 
rc => 0},
{cmd => "xen-livepatch replace xen_replace", rc => 0},
{cmd => "xen-livepatch apply xen_hello_world", rc => 256},
{cmd => "xen-livepatch apply xen_bye_world", rc => 256},
{cmd => "xl info | grep xen_extra | grep -q \"Hello Again Wor\"", rc => 
0},
{cmd => "xen-livepatch apply xen_replace", rc => 0},
{cmd => "xen-livepatch revert xen_replace", rc => 0},
{cmd => "xen-livepatch unload xen_replace", rc => 0},
{cmd => "xen-livepatch unload xen_hello_world", rc => 0},
{cmd => "xen-livepatch unload xen_bye_world", rc => 0},
{cmd => "xen-livepatch list | grep -q xen", rc => 256},
# If running this under Xen 4.4, or 5.5 it will fail.
#{cmd => "[ `xl info| grep \"xen_m\" | grep or | sed s/.*:// | uniq | 
wc -l` == 2 ]", rc => 0},
{cmd => "xen-livepatch load xen_nop.livepatch", rc => 0},
{cmd => "xen-livepatch revert xen_nop", rc => 0},
{cmd => "xen-livepatch apply xen_nop", rc => 0},
{cmd => "[ `xl info| grep \"xen_m\" | grep or | sed s/.*:// | uniq | wc 
-l` == 1 ]", rc => 0},
{cmd => "xen-livepatch unload xen_nop", rc => 256},
{cmd => "xen-livepatch revert xen_nop", rc => 0},
{cmd => "xen-livepatch unload xen_nop", rc => 0},
);

chdir("/usr/lib/debug") or die "cannot change: $!\n";

foreach my $file (@livepatch_files)
{
if ( ! -f $file ) {
die "$file is missing!\n";
}
}
print "Have $#livepatch_tests test-cases\n";
my $rc=0;
for my $i ( 0 .. $#livepatch_tests ) {
my $expected_rc = $livepatch_tests[$i]{rc};
my $cmd = $livepatch_tests[$i]{cmd};

Re: [Xen-devel] [PATCH for-4.9 v3 3/3] xen/livepatch: Don't crash on encountering STN_UNDEF relocations

[Julien Grall]

On 23/06/17 15:35, Konrad Rzeszutek Wilk wrote:

On Fri, Jun 23, 2017 at 02:45:22PM +0100, Andrew Cooper wrote:

On 23/06/17 14:43, Julien Grall wrote:

Hi,

On 23/06/17 14:33, Andrew Cooper wrote:

On 23/06/17 14:32, Julien Grall wrote:

Hi Andrew,

I am a bit confused, the title says "PATCH for-4.9 v3 3/3". I haven't
been CCed on the first two patches. Does it mean you are only looking
at this patch to be in 4.9?


Sorry - I messed up the CC lists.  The correctness of this patch does
depend on the previous two, so all 3 are looking for inclusion.


Given that we don't have livepatch testing in osstest how much test
have we done on those 3 patches?


There is testing in OSSTest.


Hurray hurray hurray!


I've manually run each of the scenarios, including with my livepatch
which has a STN_UNDEF relocation.

I don't know what testing Konrad has done.


I run a version of the same tests that are in OSSTest (basically an earlier
version of the Perl code) and I have done it on x86 and on ARM32.

And I also run the standalone OSSTest (on x86)

And then I also do a livepatch using the livepatch-build-tools on x86 to
patch some silly function.

So from a testing perspective these patches have been tested very exhaustively.


Well it has not been tested on ARM64 :). I am about to do that.

Cheers,

--
Julien Grall

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

Re: [Xen-devel] [PATCH for-4.9 v3 3/3] xen/livepatch: Don't crash on encountering STN_UNDEF relocations

[Konrad Rzeszutek Wilk]

On Fri, Jun 23, 2017 at 02:45:22PM +0100, Andrew Cooper wrote:
> On 23/06/17 14:43, Julien Grall wrote:
> > Hi,
> >
> > On 23/06/17 14:33, Andrew Cooper wrote:
> >> On 23/06/17 14:32, Julien Grall wrote:
> >>> Hi Andrew,
> >>>
> >>> I am a bit confused, the title says "PATCH for-4.9 v3 3/3". I haven't
> >>> been CCed on the first two patches. Does it mean you are only looking
> >>> at this patch to be in 4.9?
> >>
> >> Sorry - I messed up the CC lists.  The correctness of this patch does
> >> depend on the previous two, so all 3 are looking for inclusion.
> >
> > Given that we don't have livepatch testing in osstest how much test
> > have we done on those 3 patches?
> 
> There is testing in OSSTest.

Hurray hurray hurray!
> 
> I've manually run each of the scenarios, including with my livepatch
> which has a STN_UNDEF relocation.
>
> I don't know what testing Konrad has done.

I run a version of the same tests that are in OSSTest (basically an earlier
version of the Perl code) and I have done it on x86 and on ARM32.

And I also run the standalone OSSTest (on x86)

And then I also do a livepatch using the livepatch-build-tools on x86 to
patch some silly function.

So from a testing perspective these patches have been tested very exhaustively.

> 
> ~Andrew

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

Re: [Xen-devel] [PATCH for-4.9 v3 3/3] xen/livepatch: Don't crash on encountering STN_UNDEF relocations

[Julien Grall]

On 23/06/17 14:45, Andrew Cooper wrote:

On 23/06/17 14:43, Julien Grall wrote:

Hi,

On 23/06/17 14:33, Andrew Cooper wrote:

On 23/06/17 14:32, Julien Grall wrote:

Hi Andrew,

I am a bit confused, the title says "PATCH for-4.9 v3 3/3". I haven't
been CCed on the first two patches. Does it mean you are only looking
at this patch to be in 4.9?


Sorry - I messed up the CC lists.  The correctness of this patch does
depend on the previous two, so all 3 are looking for inclusion.


Given that we don't have livepatch testing in osstest how much test
have we done on those 3 patches?


There is testing in OSSTest.


Oh yes sorry. But only for amd64 and i386. No arm32 nor arm64 test.



I've manually run each of the scenarios, including with my livepatch
which has a STN_UNDEF relocation.

I don't know what testing Konrad has done.


Cheers,

--
Julien Grall

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

Re: [Xen-devel] [PATCH for-4.9 v3 3/3] xen/livepatch: Don't crash on encountering STN_UNDEF relocations

[Andrew Cooper]

On 23/06/17 14:43, Julien Grall wrote:
> Hi,
>
> On 23/06/17 14:33, Andrew Cooper wrote:
>> On 23/06/17 14:32, Julien Grall wrote:
>>> Hi Andrew,
>>>
>>> I am a bit confused, the title says "PATCH for-4.9 v3 3/3". I haven't
>>> been CCed on the first two patches. Does it mean you are only looking
>>> at this patch to be in 4.9?
>>
>> Sorry - I messed up the CC lists.  The correctness of this patch does
>> depend on the previous two, so all 3 are looking for inclusion.
>
> Given that we don't have livepatch testing in osstest how much test
> have we done on those 3 patches?

There is testing in OSSTest.

I've manually run each of the scenarios, including with my livepatch
which has a STN_UNDEF relocation.

I don't know what testing Konrad has done.

~Andrew

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

Re: [Xen-devel] [PATCH for-4.9 v3 3/3] xen/livepatch: Don't crash on encountering STN_UNDEF relocations

[Julien Grall]

Hi,

On 23/06/17 14:33, Andrew Cooper wrote:

On 23/06/17 14:32, Julien Grall wrote:

Hi Andrew,

I am a bit confused, the title says "PATCH for-4.9 v3 3/3". I haven't
been CCed on the first two patches. Does it mean you are only looking
at this patch to be in 4.9?


Sorry - I messed up the CC lists.  The correctness of this patch does
depend on the previous two, so all 3 are looking for inclusion.


Given that we don't have livepatch testing in osstest how much test have 
we done on those 3 patches?


Cheers,

--
Julien Grall

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

Re: [Xen-devel] [PATCH for-4.9 v3 3/3] xen/livepatch: Don't crash on encountering STN_UNDEF relocations

[Andrew Cooper]

On 23/06/17 14:32, Julien Grall wrote:
> Hi Andrew,
>
> I am a bit confused, the title says "PATCH for-4.9 v3 3/3". I haven't
> been CCed on the first two patches. Does it mean you are only looking
> at this patch to be in 4.9?

Sorry - I messed up the CC lists.  The correctness of this patch does
depend on the previous two, so all 3 are looking for inclusion.

~Andrew

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

Re: [Xen-devel] [PATCH for-4.9 v3 3/3] xen/livepatch: Don't crash on encountering STN_UNDEF relocations

[Julien Grall]

Hi Andrew,

I am a bit confused, the title says "PATCH for-4.9 v3 3/3". I haven't 
been CCed on the first two patches. Does it mean you are only looking at 
this patch to be in 4.9?


Cheers,

On 22/06/17 19:15, Andrew Cooper wrote:

A symndx of STN_UNDEF is special, and means a symbol value of 0.  While
legitimate in the ELF standard, its existance in a livepatch is questionable
at best.  Until a plausible usecase presents itself, reject such a relocation
with -EOPNOTSUPP.

Additionally, fix an off-by-one error while range checking symndx, and perform
a safety check on elf->sym[symndx].sym before derefencing it, to avoid
tripping over a NULL pointer when calculating val.

Signed-off-by: Andrew Cooper 
---
CC: Konrad Rzeszutek Wilk 
CC: Ross Lagerwall 
CC: Jan Beulich 
CC: Stefano Stabellini 
CC: Julien Grall 

v3:
 * Fix off-by-one error
v2:
 * Reject STN_UNDEF with -EOPNOTSUPP
---
 xen/arch/arm/arm32/livepatch.c | 14 +-
 xen/arch/arm/arm64/livepatch.c | 14 +-
 xen/arch/x86/livepatch.c   | 14 +-
 3 files changed, 39 insertions(+), 3 deletions(-)

diff --git a/xen/arch/arm/arm32/livepatch.c b/xen/arch/arm/arm32/livepatch.c
index a328179..41378a5 100644
--- a/xen/arch/arm/arm32/livepatch.c
+++ b/xen/arch/arm/arm32/livepatch.c
@@ -254,12 +254,24 @@ int arch_livepatch_perform(struct livepatch_elf *elf,
 addend = get_addend(type, dest);
 }

-if ( symndx > elf->nsym )
+if ( symndx == STN_UNDEF )
+{
+dprintk(XENLOG_ERR, LIVEPATCH "%s: Encountered STN_UNDEF\n",
+elf->name);
+return -EOPNOTSUPP;
+}
+else if ( symndx >= elf->nsym )
 {
 dprintk(XENLOG_ERR, LIVEPATCH "%s: Relative symbol wants symbol@%u 
which is past end!\n",
 elf->name, symndx);
 return -EINVAL;
 }
+else if ( !elf->sym[symndx].sym )
+{
+dprintk(XENLOG_ERR, LIVEPATCH "%s: No relative symbol@%u\n",
+elf->name, symndx);
+return -EINVAL;
+}

 val = elf->sym[symndx].sym->st_value; /* S */

diff --git a/xen/arch/arm/arm64/livepatch.c b/xen/arch/arm/arm64/livepatch.c
index 63929b1..2247b92 100644
--- a/xen/arch/arm/arm64/livepatch.c
+++ b/xen/arch/arm/arm64/livepatch.c
@@ -252,12 +252,24 @@ int arch_livepatch_perform_rela(struct livepatch_elf *elf,
 int ovf = 0;
 uint64_t val;

-if ( symndx > elf->nsym )
+if ( symndx == STN_UNDEF )
+{
+dprintk(XENLOG_ERR, LIVEPATCH "%s: Encountered STN_UNDEF\n",
+elf->name);
+return -EOPNOTSUPP;
+}
+else if ( symndx >= elf->nsym )
 {
 dprintk(XENLOG_ERR, LIVEPATCH "%s: Relative relocation wants symbol@%u 
which is past end!\n",
 elf->name, symndx);
 return -EINVAL;
 }
+else if ( !elf->sym[symndx].sym )
+{
+dprintk(XENLOG_ERR, LIVEPATCH "%s: No relative symbol@%u\n",
+elf->name, symndx);
+return -EINVAL;
+}

 val = elf->sym[symndx].sym->st_value +  r->r_addend; /* S+A */

diff --git a/xen/arch/x86/livepatch.c b/xen/arch/x86/livepatch.c
index 7917610..406eb91 100644
--- a/xen/arch/x86/livepatch.c
+++ b/xen/arch/x86/livepatch.c
@@ -170,12 +170,24 @@ int arch_livepatch_perform_rela(struct livepatch_elf *elf,
 uint8_t *dest = base->load_addr + r->r_offset;
 uint64_t val;

-if ( symndx > elf->nsym )
+if ( symndx == STN_UNDEF )
+{
+dprintk(XENLOG_ERR, LIVEPATCH "%s: Encountered STN_UNDEF\n",
+elf->name);
+return -EOPNOTSUPP;
+}
+else if ( symndx >= elf->nsym )
 {
 dprintk(XENLOG_ERR, LIVEPATCH "%s: Relative relocation wants symbol@%u 
which is past end!\n",
 elf->name, symndx);
 return -EINVAL;
 }
+else if ( !elf->sym[symndx].sym )
+{
+dprintk(XENLOG_ERR, LIVEPATCH "%s: No symbol@%u\n",
+elf->name, symndx);
+return -EINVAL;
+}

 val = r->r_addend + elf->sym[symndx].sym->st_value;




--
Julien Grall

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

Re: [Xen-devel] [PATCH for-4.9 v3 3/3] xen/livepatch: Don't crash on encountering STN_UNDEF relocations

[Ross Lagerwall]

On 06/22/2017 07:15 PM, Andrew Cooper wrote:

A symndx of STN_UNDEF is special, and means a symbol value of 0.  While
legitimate in the ELF standard, its existance in a livepatch is questionable
at best.  Until a plausible usecase presents itself, reject such a relocation
with -EOPNOTSUPP.

Additionally, fix an off-by-one error while range checking symndx, and perform
a safety check on elf->sym[symndx].sym before derefencing it, to avoid
tripping over a NULL pointer when calculating val.

Signed-off-by: Andrew Cooper 


Reviewed-by: Ross Lagerwall 

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

Re: [Xen-devel] [PATCH for-4.9 v3 3/3] xen/livepatch: Don't crash on encountering STN_UNDEF relocations

[Andrew Cooper]

On 23/06/17 10:50, Jan Beulich wrote:
 On 22.06.17 at 20:15,  wrote:
>> A symndx of STN_UNDEF is special, and means a symbol value of 0.  While
>> legitimate in the ELF standard, its existance in a livepatch is questionable
>> at best.  Until a plausible usecase presents itself, reject such a relocation
>> with -EOPNOTSUPP.
>>
>> Additionally, fix an off-by-one error while range checking symndx, and 
>> perform
>> a safety check on elf->sym[symndx].sym before derefencing it, to avoid
>> tripping over a NULL pointer when calculating val.
>>
>> Signed-off-by: Andrew Cooper 
> Reviewed-by: Jan Beulich 
> with two remarks:
>
>> --- a/xen/arch/x86/livepatch.c
>> +++ b/xen/arch/x86/livepatch.c
>> @@ -170,12 +170,24 @@ int arch_livepatch_perform_rela(struct livepatch_elf 
>> *elf,
>>  uint8_t *dest = base->load_addr + r->r_offset;
>>  uint64_t val;
>>  
>> -if ( symndx > elf->nsym )
>> +if ( symndx == STN_UNDEF )
>> +{
>> +dprintk(XENLOG_ERR, LIVEPATCH "%s: Encountered STN_UNDEF\n",
>> +elf->name);
>> +return -EOPNOTSUPP;
>> +}
>> +else if ( symndx >= elf->nsym )
>>  {
>>  dprintk(XENLOG_ERR, LIVEPATCH "%s: Relative relocation wants 
>> symbol@%u which is past end!\n",
>>  elf->name, symndx);
>>  return -EINVAL;
>>  }
>> +else if ( !elf->sym[symndx].sym )
> Neither of the two "else" is really necessary, and elsewhere we've
> been telling people to avoid such.

I see two logically different scenarios.

Per the style, if I were to use fully separate if() statements, I'd need
a newline between each.  This expands the code, and separates a chain of
logically-related checks.

IMO, its better to keep logically related checks more obviously
together, while I would definitely agree that unrelated chains (which
could in principle be if/else like this) should be separated.

>
>> +{
>> +dprintk(XENLOG_ERR, LIVEPATCH "%s: No symbol@%u\n",
> Symbol tables can grow large, and for large numbers I generally
> find hex representation preferable of dec. Otoh the other
> (pre-existing) message uses dec too ...

I'll stay consistent with everything else.

~Andrew

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

Re: [Xen-devel] [PATCH for-4.9 v3 3/3] xen/livepatch: Don't crash on encountering STN_UNDEF relocations

[Jan Beulich]

>>> On 22.06.17 at 20:15,  wrote:
> A symndx of STN_UNDEF is special, and means a symbol value of 0.  While
> legitimate in the ELF standard, its existance in a livepatch is questionable
> at best.  Until a plausible usecase presents itself, reject such a relocation
> with -EOPNOTSUPP.
> 
> Additionally, fix an off-by-one error while range checking symndx, and perform
> a safety check on elf->sym[symndx].sym before derefencing it, to avoid
> tripping over a NULL pointer when calculating val.
> 
> Signed-off-by: Andrew Cooper 

Reviewed-by: Jan Beulich 
with two remarks:

> --- a/xen/arch/x86/livepatch.c
> +++ b/xen/arch/x86/livepatch.c
> @@ -170,12 +170,24 @@ int arch_livepatch_perform_rela(struct livepatch_elf 
> *elf,
>  uint8_t *dest = base->load_addr + r->r_offset;
>  uint64_t val;
>  
> -if ( symndx > elf->nsym )
> +if ( symndx == STN_UNDEF )
> +{
> +dprintk(XENLOG_ERR, LIVEPATCH "%s: Encountered STN_UNDEF\n",
> +elf->name);
> +return -EOPNOTSUPP;
> +}
> +else if ( symndx >= elf->nsym )
>  {
>  dprintk(XENLOG_ERR, LIVEPATCH "%s: Relative relocation wants 
> symbol@%u which is past end!\n",
>  elf->name, symndx);
>  return -EINVAL;
>  }
> +else if ( !elf->sym[symndx].sym )

Neither of the two "else" is really necessary, and elsewhere we've
been telling people to avoid such.

> +{
> +dprintk(XENLOG_ERR, LIVEPATCH "%s: No symbol@%u\n",

Symbol tables can grow large, and for large numbers I generally
find hex representation preferable of dec. Otoh the other
(pre-existing) message uses dec too ...

Jan


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

Re: [Xen-devel] [PATCH for-4.9 v3 3/3] xen/livepatch: Don't crash on encountering STN_UNDEF relocations

[Konrad Rzeszutek Wilk]

On Thu, Jun 22, 2017 at 07:15:29PM +0100, Andrew Cooper wrote:
> A symndx of STN_UNDEF is special, and means a symbol value of 0.  While
> legitimate in the ELF standard, its existance in a livepatch is questionable
> at best.  Until a plausible usecase presents itself, reject such a relocation
> with -EOPNOTSUPP.
> 
> Additionally, fix an off-by-one error while range checking symndx, and perform
> a safety check on elf->sym[symndx].sym before derefencing it, to avoid
> tripping over a NULL pointer when calculating val.
> 
> Signed-off-by: Andrew Cooper 
> ---
> CC: Konrad Rzeszutek Wilk 

Reviewed-by: Konrad Rzeszutek Wilk 
Tested-by: Konrad Rzeszutek Wilk  [arm32 and x86]

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

Re: [Xen-devel] [PATCH for-4.9 v3 3/3] xen/livepatch: Don't crash on encountering STN_UNDEF relocations

[Stefano Stabellini]

On Thu, 22 Jun 2017, Andrew Cooper wrote:
> A symndx of STN_UNDEF is special, and means a symbol value of 0.  While
> legitimate in the ELF standard, its existance in a livepatch is questionable
> at best.  Until a plausible usecase presents itself, reject such a relocation
> with -EOPNOTSUPP.
> 
> Additionally, fix an off-by-one error while range checking symndx, and perform
> a safety check on elf->sym[symndx].sym before derefencing it, to avoid
> tripping over a NULL pointer when calculating val.
> 
> Signed-off-by: Andrew Cooper 
> ---
> CC: Konrad Rzeszutek Wilk 
> CC: Ross Lagerwall 
> CC: Jan Beulich 
> CC: Stefano Stabellini 
> CC: Julien Grall 
> 
> v3:
>  * Fix off-by-one error
> v2:
>  * Reject STN_UNDEF with -EOPNOTSUPP

Reviewed-by: Stefano Stabellini 


> ---
>  xen/arch/arm/arm32/livepatch.c | 14 +-
>  xen/arch/arm/arm64/livepatch.c | 14 +-
>  xen/arch/x86/livepatch.c   | 14 +-
>  3 files changed, 39 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/arm/arm32/livepatch.c b/xen/arch/arm/arm32/livepatch.c
> index a328179..41378a5 100644
> --- a/xen/arch/arm/arm32/livepatch.c
> +++ b/xen/arch/arm/arm32/livepatch.c
> @@ -254,12 +254,24 @@ int arch_livepatch_perform(struct livepatch_elf *elf,
>  addend = get_addend(type, dest);
>  }
>  
> -if ( symndx > elf->nsym )
> +if ( symndx == STN_UNDEF )
> +{
> +dprintk(XENLOG_ERR, LIVEPATCH "%s: Encountered STN_UNDEF\n",
> +elf->name);
> +return -EOPNOTSUPP;
> +}
> +else if ( symndx >= elf->nsym )
>  {
>  dprintk(XENLOG_ERR, LIVEPATCH "%s: Relative symbol wants 
> symbol@%u which is past end!\n",
>  elf->name, symndx);
>  return -EINVAL;
>  }
> +else if ( !elf->sym[symndx].sym )
> +{
> +dprintk(XENLOG_ERR, LIVEPATCH "%s: No relative symbol@%u\n",
> +elf->name, symndx);
> +return -EINVAL;
> +}
>  
>  val = elf->sym[symndx].sym->st_value; /* S */
>  
> diff --git a/xen/arch/arm/arm64/livepatch.c b/xen/arch/arm/arm64/livepatch.c
> index 63929b1..2247b92 100644
> --- a/xen/arch/arm/arm64/livepatch.c
> +++ b/xen/arch/arm/arm64/livepatch.c
> @@ -252,12 +252,24 @@ int arch_livepatch_perform_rela(struct livepatch_elf 
> *elf,
>  int ovf = 0;
>  uint64_t val;
>  
> -if ( symndx > elf->nsym )
> +if ( symndx == STN_UNDEF )
> +{
> +dprintk(XENLOG_ERR, LIVEPATCH "%s: Encountered STN_UNDEF\n",
> +elf->name);
> +return -EOPNOTSUPP;
> +}
> +else if ( symndx >= elf->nsym )
>  {
>  dprintk(XENLOG_ERR, LIVEPATCH "%s: Relative relocation wants 
> symbol@%u which is past end!\n",
>  elf->name, symndx);
>  return -EINVAL;
>  }
> +else if ( !elf->sym[symndx].sym )
> +{
> +dprintk(XENLOG_ERR, LIVEPATCH "%s: No relative symbol@%u\n",
> +elf->name, symndx);
> +return -EINVAL;
> +}
>  
>  val = elf->sym[symndx].sym->st_value +  r->r_addend; /* S+A */
>  
> diff --git a/xen/arch/x86/livepatch.c b/xen/arch/x86/livepatch.c
> index 7917610..406eb91 100644
> --- a/xen/arch/x86/livepatch.c
> +++ b/xen/arch/x86/livepatch.c
> @@ -170,12 +170,24 @@ int arch_livepatch_perform_rela(struct livepatch_elf 
> *elf,
>  uint8_t *dest = base->load_addr + r->r_offset;
>  uint64_t val;
>  
> -if ( symndx > elf->nsym )
> +if ( symndx == STN_UNDEF )
> +{
> +dprintk(XENLOG_ERR, LIVEPATCH "%s: Encountered STN_UNDEF\n",
> +elf->name);
> +return -EOPNOTSUPP;
> +}
> +else if ( symndx >= elf->nsym )
>  {
>  dprintk(XENLOG_ERR, LIVEPATCH "%s: Relative relocation wants 
> symbol@%u which is past end!\n",
>  elf->name, symndx);
>  return -EINVAL;
>  }
> +else if ( !elf->sym[symndx].sym )
> +{
> +dprintk(XENLOG_ERR, LIVEPATCH "%s: No symbol@%u\n",
> +elf->name, symndx);
> +return -EINVAL;
> +}
>  
>  val = r->r_addend + elf->sym[symndx].sym->st_value;
>  
> -- 
> 2.1.4
> 

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