Discussion:
[PATCH] libelf: Fix div0 issues in elf_{shdr, phdr}_count()
(too old to reply)
Andrew Cooper
2016-12-08 14:18:58 UTC
Permalink
elf_uval() can return zero either because the field itself is zero, or because
the access is out of bounds.

c/s a01b6d4 "libelf: treat phdr and shdr similarly" introduced two div0 issues
as e_{ph,sh}entsize are not checked for sanity before being used to divide
elf->size.

Spotted by Coverity.

Signed-off-by: Andrew Cooper <***@citrix.com>
---
CC: George Dunlap <***@eu.citrix.com>
CC: Ian Jackson <***@eu.citrix.com>
CC: Jan Beulich <***@suse.com>
CC: Konrad Rzeszutek Wilk <***@oracle.com>
CC: Stefano Stabellini <***@kernel.org>
CC: Tim Deegan <***@xen.org>
CC: Wei Liu <***@citrix.com>

I experimented with making elf_access_unsigned() __must_check, but this didn't
cause a compiler error. I am not quite sure why.
---
xen/common/libelf/libelf-tools.c | 16 ++++++++++++++--
1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/xen/common/libelf/libelf-tools.c b/xen/common/libelf/libelf-tools.c
index bf661e7..f62d9c3 100644
--- a/xen/common/libelf/libelf-tools.c
+++ b/xen/common/libelf/libelf-tools.c
@@ -130,11 +130,17 @@ uint64_t elf_round_up(struct elf_binary *elf, uint64_t addr)
unsigned elf_shdr_count(struct elf_binary *elf)
{
unsigned count = elf_uval(elf, elf->ehdr, e_shnum);
+ unsigned entsize = elf_uval(elf, elf->ehdr, e_shentsize);
uint64_t max;

if ( !count )
return 0;
- max = elf->size / elf_uval(elf, elf->ehdr, e_shentsize);
+ if ( !entsize )
+ {
+ elf_mark_broken(elf, "e_shentsize is zero");
+ return 0;
+ }
+ max = elf->size / entsize;
if ( max > UINT_MAX )
max = UINT_MAX;
if ( count > max )
@@ -148,11 +154,17 @@ unsigned elf_shdr_count(struct elf_binary *elf)
unsigned elf_phdr_count(struct elf_binary *elf)
{
unsigned count = elf_uval(elf, elf->ehdr, e_phnum);
+ unsigned entsize = elf_uval(elf, elf->ehdr, e_phentsize);
uint64_t max;

if ( !count )
return 0;
- max = elf->size / elf_uval(elf, elf->ehdr, e_phentsize);
+ if ( !entsize )
+ {
+ elf_mark_broken(elf, "e_phentsize is zero");
+ return 0;
+ }
+ max = elf->size / entsize;
if ( max > UINT_MAX )
max = UINT_MAX;
if ( count > max )
--
2.1.4
Jan Beulich
2016-12-08 14:41:29 UTC
Permalink
Post by Andrew Cooper
elf_uval() can return zero either because the field itself is zero, or because
the access is out of bounds.
c/s a01b6d4 "libelf: treat phdr and shdr similarly" introduced two div0 issues
as e_{ph,sh}entsize are not checked for sanity before being used to divide
elf->size.
Spotted by Coverity.
And wrongly so, imo.
Post by Andrew Cooper
--- a/xen/common/libelf/libelf-tools.c
+++ b/xen/common/libelf/libelf-tools.c
@@ -130,11 +130,17 @@ uint64_t elf_round_up(struct elf_binary *elf, uint64_t addr)
unsigned elf_shdr_count(struct elf_binary *elf)
{
unsigned count = elf_uval(elf, elf->ehdr, e_shnum);
+ unsigned entsize = elf_uval(elf, elf->ehdr, e_shentsize);
uint64_t max;
if ( !count )
return 0;
- max = elf->size / elf_uval(elf, elf->ehdr, e_shentsize);
+ if ( !entsize )
+ {
+ elf_mark_broken(elf, "e_shentsize is zero");
+ return 0;
+ }
This as well as ...
Post by Andrew Cooper
@@ -148,11 +154,17 @@ unsigned elf_shdr_count(struct elf_binary *elf)
unsigned elf_phdr_count(struct elf_binary *elf)
{
unsigned count = elf_uval(elf, elf->ehdr, e_phnum);
+ unsigned entsize = elf_uval(elf, elf->ehdr, e_phentsize);
uint64_t max;
if ( !count )
return 0;
- max = elf->size / elf_uval(elf, elf->ehdr, e_phentsize);
+ if ( !entsize )
+ {
+ elf_mark_broken(elf, "e_phentsize is zero");
+ return 0;
+ }
... this would end up being dead code, due to the checks the same
patch you refer to introduced in elf_init().

Jan
Andrew Cooper
2016-12-08 14:46:48 UTC
Permalink
Post by Jan Beulich
Post by Andrew Cooper
elf_uval() can return zero either because the field itself is zero, or because
the access is out of bounds.
c/s a01b6d4 "libelf: treat phdr and shdr similarly" introduced two div0 issues
as e_{ph,sh}entsize are not checked for sanity before being used to divide
elf->size.
Spotted by Coverity.
And wrongly so, imo.
Post by Andrew Cooper
--- a/xen/common/libelf/libelf-tools.c
+++ b/xen/common/libelf/libelf-tools.c
@@ -130,11 +130,17 @@ uint64_t elf_round_up(struct elf_binary *elf, uint64_t addr)
unsigned elf_shdr_count(struct elf_binary *elf)
{
unsigned count = elf_uval(elf, elf->ehdr, e_shnum);
+ unsigned entsize = elf_uval(elf, elf->ehdr, e_shentsize);
uint64_t max;
if ( !count )
return 0;
- max = elf->size / elf_uval(elf, elf->ehdr, e_shentsize);
+ if ( !entsize )
+ {
+ elf_mark_broken(elf, "e_shentsize is zero");
+ return 0;
+ }
This as well as ...
Post by Andrew Cooper
@@ -148,11 +154,17 @@ unsigned elf_shdr_count(struct elf_binary *elf)
unsigned elf_phdr_count(struct elf_binary *elf)
{
unsigned count = elf_uval(elf, elf->ehdr, e_phnum);
+ unsigned entsize = elf_uval(elf, elf->ehdr, e_phentsize);
uint64_t max;
if ( !count )
return 0;
- max = elf->size / elf_uval(elf, elf->ehdr, e_phentsize);
+ if ( !entsize )
+ {
+ elf_mark_broken(elf, "e_phentsize is zero");
+ return 0;
+ }
... this would end up being dead code, due to the checks the same
patch you refer to introduced in elf_init().
Are you sure? elf_init() currently looks like this:

/* Sanity check phdr if present. */
count = elf_phdr_count(elf);
if ( count )
{
if ( elf_uval(elf, elf->ehdr, e_phentsize) <
elf_size(elf, ELF_HANDLE_DECL(elf_phdr)) )
{
elf_err(elf, "ELF: phdr too small (%" PRIu64 ")\n",
elf_uval(elf, elf->ehdr, e_phentsize));
return -1;
}

There is no check of e_phentsize before it is used to divide with.

~Andrew
Jan Beulich
2016-12-08 15:17:35 UTC
Permalink
Post by Andrew Cooper
Post by Jan Beulich
Post by Andrew Cooper
elf_uval() can return zero either because the field itself is zero, or
because
Post by Jan Beulich
Post by Andrew Cooper
the access is out of bounds.
c/s a01b6d4 "libelf: treat phdr and shdr similarly" introduced two div0
issues
Post by Jan Beulich
Post by Andrew Cooper
as e_{ph,sh}entsize are not checked for sanity before being used to divide
elf->size.
Spotted by Coverity.
And wrongly so, imo.
Post by Andrew Cooper
--- a/xen/common/libelf/libelf-tools.c
+++ b/xen/common/libelf/libelf-tools.c
@@ -130,11 +130,17 @@ uint64_t elf_round_up(struct elf_binary *elf, uint64_t
addr)
Post by Jan Beulich
Post by Andrew Cooper
unsigned elf_shdr_count(struct elf_binary *elf)
{
unsigned count = elf_uval(elf, elf->ehdr, e_shnum);
+ unsigned entsize = elf_uval(elf, elf->ehdr, e_shentsize);
uint64_t max;
if ( !count )
return 0;
- max = elf->size / elf_uval(elf, elf->ehdr, e_shentsize);
+ if ( !entsize )
+ {
+ elf_mark_broken(elf, "e_shentsize is zero");
+ return 0;
+ }
This as well as ...
Post by Andrew Cooper
@@ -148,11 +154,17 @@ unsigned elf_shdr_count(struct elf_binary *elf)
unsigned elf_phdr_count(struct elf_binary *elf)
{
unsigned count = elf_uval(elf, elf->ehdr, e_phnum);
+ unsigned entsize = elf_uval(elf, elf->ehdr, e_phentsize);
uint64_t max;
if ( !count )
return 0;
- max = elf->size / elf_uval(elf, elf->ehdr, e_phentsize);
+ if ( !entsize )
+ {
+ elf_mark_broken(elf, "e_phentsize is zero");
+ return 0;
+ }
... this would end up being dead code, due to the checks the same
patch you refer to introduced in elf_init().
/* Sanity check phdr if present. */
count = elf_phdr_count(elf);
if ( count )
{
if ( elf_uval(elf, elf->ehdr, e_phentsize) <
elf_size(elf, ELF_HANDLE_DECL(elf_phdr)) )
{
elf_err(elf, "ELF: phdr too small (%" PRIu64 ")\n",
elf_uval(elf, elf->ehdr, e_phentsize));
return -1;
}
There is no check of e_phentsize before it is used to divide with.
Oh, I see - elf_phdr_count() itself relies on the check that's
about to be done. But I think the correct thing then would be to
not use elf_phdr_count() here, but read the raw field instead.
You patch basically adds a weaker check there which is then
immediately being followed by the stronger check above.

And looking at it from that angle it's then questionable why
elf_{p,s}hdr_count() do any checking at all - this should all be
done once in elf_init(). IOW I did the adjustment in the wrong
way, and I really should have made elf_shdr_count() match
elf_phdr_count() (moving the checks into elf_init()).

And looking at elf_init() again I also realize that I blindly
copied the range checks, calculation of which can overflow.
I think it would be better to do those checks such that
overflow results in an error return.

I wonder if it wouldn't be better to revert that change, for
me to produce a better v2.

Jan
Andrew Cooper
2016-12-08 15:23:28 UTC
Permalink
Post by Jan Beulich
Post by Andrew Cooper
Post by Jan Beulich
Post by Andrew Cooper
elf_uval() can return zero either because the field itself is zero, or
because
Post by Jan Beulich
Post by Andrew Cooper
the access is out of bounds.
c/s a01b6d4 "libelf: treat phdr and shdr similarly" introduced two div0
issues
Post by Jan Beulich
Post by Andrew Cooper
as e_{ph,sh}entsize are not checked for sanity before being used to divide
elf->size.
Spotted by Coverity.
And wrongly so, imo.
Post by Andrew Cooper
--- a/xen/common/libelf/libelf-tools.c
+++ b/xen/common/libelf/libelf-tools.c
@@ -130,11 +130,17 @@ uint64_t elf_round_up(struct elf_binary *elf, uint64_t
addr)
Post by Jan Beulich
Post by Andrew Cooper
unsigned elf_shdr_count(struct elf_binary *elf)
{
unsigned count = elf_uval(elf, elf->ehdr, e_shnum);
+ unsigned entsize = elf_uval(elf, elf->ehdr, e_shentsize);
uint64_t max;
if ( !count )
return 0;
- max = elf->size / elf_uval(elf, elf->ehdr, e_shentsize);
+ if ( !entsize )
+ {
+ elf_mark_broken(elf, "e_shentsize is zero");
+ return 0;
+ }
This as well as ...
Post by Andrew Cooper
@@ -148,11 +154,17 @@ unsigned elf_shdr_count(struct elf_binary *elf)
unsigned elf_phdr_count(struct elf_binary *elf)
{
unsigned count = elf_uval(elf, elf->ehdr, e_phnum);
+ unsigned entsize = elf_uval(elf, elf->ehdr, e_phentsize);
uint64_t max;
if ( !count )
return 0;
- max = elf->size / elf_uval(elf, elf->ehdr, e_phentsize);
+ if ( !entsize )
+ {
+ elf_mark_broken(elf, "e_phentsize is zero");
+ return 0;
+ }
... this would end up being dead code, due to the checks the same
patch you refer to introduced in elf_init().
/* Sanity check phdr if present. */
count = elf_phdr_count(elf);
if ( count )
{
if ( elf_uval(elf, elf->ehdr, e_phentsize) <
elf_size(elf, ELF_HANDLE_DECL(elf_phdr)) )
{
elf_err(elf, "ELF: phdr too small (%" PRIu64 ")\n",
elf_uval(elf, elf->ehdr, e_phentsize));
return -1;
}
There is no check of e_phentsize before it is used to divide with.
Oh, I see - elf_phdr_count() itself relies on the check that's
about to be done. But I think the correct thing then would be to
not use elf_phdr_count() here, but read the raw field instead.
You patch basically adds a weaker check there which is then
immediately being followed by the stronger check above.
And looking at it from that angle it's then questionable why
elf_{p,s}hdr_count() do any checking at all - this should all be
done once in elf_init(). IOW I did the adjustment in the wrong
way, and I really should have made elf_shdr_count() match
elf_phdr_count() (moving the checks into elf_init()).
And looking at elf_init() again I also realize that I blindly
copied the range checks, calculation of which can overflow.
I think it would be better to do those checks such that
overflow results in an error return.
I wonder if it wouldn't be better to revert that change, for
me to produce a better v2.
Feel free.

I have to admit that I find it odd that the values aren't sanitised
once, then copied out into local good state. The repeated use of
elf_uval() risks a lot of zeroes because of its range check case.

~Andrew
Ian Jackson
2016-12-08 15:47:40 UTC
Permalink
Post by Jan Beulich
Oh, I see - elf_phdr_count() itself relies on the check that's
about to be done. But I think the correct thing then would be to
not use elf_phdr_count() here, but read the raw field instead.
You patch basically adds a weaker check there which is then
immediately being followed by the stronger check above.
There must be no "reading of raw fields". You may use
elf_access_unsigned if you really want to.
Post by Jan Beulich
And looking at it from that angle it's then questionable why
elf_{p,s}hdr_count() do any checking at all - this should all be
done once in elf_init(). IOW I did the adjustment in the wrong
way, and I really should have made elf_shdr_count() match
elf_phdr_count() (moving the checks into elf_init()).
The point of this checking is not to avoid overflow, but just to
ensure that the loops which rely on elf_*_count do not iterate "far
too much".
Post by Jan Beulich
And looking at elf_init() again I also realize that I blindly
copied the range checks, calculation of which can overflow.
These possibly-faulty range checks are harmless because they all
operate on unsigned values.

Frankly I'm not sure what the point of a01b6d46 was ?
Post by Jan Beulich
I think it would be better to do those checks such that
overflow results in an error return.
The design principle behind my fixes to XSA-55 is to write the bulk of
libelf in a subset of C which is always safe.

This means:

* Always using unsigned integers (so no signed integer overflow).

* Doing all pointer dereferences into the ELF block using an
access function which does a bounds check. (And this also
means representing all pointers as unsigned offsets, so that we
avoid calculating basilisk pointer values.)

* Explicitly limiting the iteration count of every loop where
necessayr (to avoid DOS attacks from bad images).

* Not using unsafe operations like division by input-controlled
values.

Sticking to these rules is a lot easier than writing explicit checks.
This is because these explicit checks are very easy to omit, or get
wrong. The pointers are all encapsulated in a special type which
prevents uncontrolled dereference.

Admittedly the rule about iteration count is not so easy to remember,
and I had failed to anticipate that someone would introduce division.
But both of those kind of bugs are denial of service, rather than
privilege escalation.

Having stared at the commit message of a01b6d46 and at the
pre-a01b6d46 code, I still don't understand what motivated the
changes.

Ian.
Jan Beulich
2016-12-08 16:09:59 UTC
Permalink
Andrew Cooper writes ("Re: [PATCH] libelf: Fix div0 issues in
Post by Jan Beulich
Oh, I see - elf_phdr_count() itself relies on the check that's
about to be done. But I think the correct thing then would be to
not use elf_phdr_count() here, but read the raw field instead.
You patch basically adds a weaker check there which is then
immediately being followed by the stronger check above.
There must be no "reading of raw fields". You may use
elf_access_unsigned if you really want to.
Oh, by "raw" I meant using elf_uval() rather than elf_phdr_count().
Post by Jan Beulich
And looking at it from that angle it's then questionable why
elf_{p,s}hdr_count() do any checking at all - this should all be
done once in elf_init(). IOW I did the adjustment in the wrong
way, and I really should have made elf_shdr_count() match
elf_phdr_count() (moving the checks into elf_init()).
The point of this checking is not to avoid overflow, but just to
ensure that the loops which rely on elf_*_count do not iterate "far
too much".
Post by Jan Beulich
And looking at elf_init() again I also realize that I blindly
copied the range checks, calculation of which can overflow.
These possibly-faulty range checks are harmless because they all
operate on unsigned values.
Why does operating on unsigned values make overflow harmless?
Frankly I'm not sure what the point of a01b6d46 was ?
First of all I'd like to refer you to its description, but see below.
Are you suggesting that all those adjustments appear pointless
to you?
* Not using unsafe operations like division by input-controlled
values.
But using a hard coded but possibly wrong value instead isn't
really a solution.
Having stared at the commit message of a01b6d46 and at the
pre-a01b6d46 code, I still don't understand what motivated the
changes.
Hmm, that's interesting. Are you then saying all the changes it
did and described are wrong? Pointless? I could do with some
clarification here, as otherwise it's not clear whether spending
time on producing an improved v2 is actually worth it.

Jan
Ian Jackson
2016-12-08 17:28:51 UTC
Permalink
Post by Ian Jackson
Admittedly the rule about iteration count is not so easy to remember,
and I had failed to anticipate that someone would introduce division.
But both of those kind of bugs are denial of service, rather than
privilege escalation.
Having stared at the commit message of a01b6d46 and at the
pre-a01b6d46 code, I still don't understand what motivated the
changes.
Jan and I had a conversation on irc.


Part of the motivation for a01b6d4 was the anomaly which is the
difference between the implementations of elf_phdr_size and
elf_shdr_size.

This was introduced in 39bf7b9d0ae5 "libelf: check loops for running
away". That was part of the XSA-55 patchset.

Looking at the commit message I was concerned that the phdr and shdr
counts might be excessive, and that libelf might get stuck in a loop
with an unreasonably high iteration count.

I appear to have attempted to mitigate this by:

(a) in the case of shdrs, by using elf_access_ok inside each
loop, on the principle that an out of control loop count
would walk off the end of the image and stop;

(b) in the case of phdrs, making an explicit limit of
(image size) / sizeof(phdr). NB that sizeof(phdr) is not the
actual size of phdrs; for a valid ELF it is a minimum. This
is fine because we're computing a backstop, which does not
have to be entirely accurate.

I now see that (a) is ineffective because the image can specify its
own stride for the iteration (the header size). If it specifies a
stride of zero, the maximum count is unbounded.

However, both count values are actually 16-bit. So there is already a
limit. The extra code in elf_phdr_size is not necessary.

IMO this near-miss demonstrates that a more robust approach is
required to bounding loops in libelf.

A possibility would be to introduce
bool elf_iter_cont(struct elf_binary *elf, size_t copysz);
and using it in every for(;;) in libelf. It would keep a counter (set
to zero in libelf) and abandon the processing if the total of all the
copysz values passed was "too large". (copysz would be bounded below
by 1, so that it is safe to pass a size from the ELF.)

Then

- for ( i = 0; i < elf_shdr_count(elf); i++ )
+ for ( i = 0; elf_iter_cont(elf, e_shentsize)
+ && i < elf_shdr_count(elf); i++ )

An alternative would be to decorate every loop with a comment
explaining why the loop does reasonably-bounded work. I'm not sure
whether this, or my more sophisticated suggestion, will find favour.

In any case, the max calculation in elf_phdr_size is unnecessary for
security, and has no purpose related to functionality, and can be
deleted.


Another part of the motivation for a01b6d4 is to produce better
messages for certain malformed ELFs. There has been no assertion that
any such ELFs (ie ELFs which would fail these new checks) actually
exist or have caused trouble for anyone.

I think that it is a bad idea to add unnecessary checking to libelf.

libelf is a format parser on a security boundary. Despite attempts to
provide a safe dialect to write in, every piece of code in libelf
risks security problems.

If libelf "accepts" some malformed image, and constructs a mess in the
guest memory space, then that is unfortunate. But it is not a big
problem. The guest will probably crash or go wrong, but that is not a
problem for the host.

If someone is faced with fallout from a malformed ELF, they would
hopefully try use an object file inspector like objdump on the loaded
image. That would reveal the problem. (And of course ELFs are mostly
generated by a fairly small ecosystem of toolchain programs.)


Also, this has demonstrated that the design principles underlying the
safety of libelf are not properly understood. IIRC they were
explained in the XSA-55 00/ patch, but that's not sufficient.


So I would prefer to:


Revert all of a01b6d4.


Delete the header count check in elf_phdr_count and replace it with a
compile-time assertion, in elf_phdr_count and in elf_shdr_count, that
sizeof the count field is <= 2. (This may need a new macro
ELF_HANDLE_FIELD_SIZEOF implemented in terms of
ELF__HANDLE_FIELD_TYPE.)


Add a comment near the top of libelf.h explaining the rules for
programming inside libelf, to include:

* Arithmetic on signed values is forbidden

* Use of actual pointers into the ELF is forbidden

* Division by non-constant values is forbidden

* All loops must ensure that they have a reasonably low
iteration count

* Code (including ELF format sanity checks) which is necessary
neither for safety nor for correct processing of correct files
should not be added to libelf. It is not libelf's role to
be an ELF validator.


Thanks,
Ian.
Jan Beulich
2016-12-09 08:38:46 UTC
Permalink
Post by Ian Jackson
Part of the motivation for a01b6d4 was the anomaly which is the
difference between the implementations of elf_phdr_size and
elf_shdr_size.
This was introduced in 39bf7b9d0ae5 "libelf: check loops for running
away". That was part of the XSA-55 patchset.
Looking at the commit message I was concerned that the phdr and shdr
counts might be excessive, and that libelf might get stuck in a loop
with an unreasonably high iteration count.
Looking at that commit message, what I'm missing first of all is an
explanation of why long loops are a problem in the first place. Do
we need to be concerned of single threaded tool stacks? I think
such a problem would better be addressed at higher layers,
switching to multi threaded designs.
Post by Ian Jackson
(a) in the case of shdrs, by using elf_access_ok inside each
loop, on the principle that an out of control loop count
would walk off the end of the image and stop;
(b) in the case of phdrs, making an explicit limit of
(image size) / sizeof(phdr). NB that sizeof(phdr) is not the
actual size of phdrs; for a valid ELF it is a minimum. This
is fine because we're computing a backstop, which does not
have to be entirely accurate.
I now see that (a) is ineffective because the image can specify its
own stride for the iteration (the header size). If it specifies a
stride of zero, the maximum count is unbounded.
Well, a stride of zero is invalid (as is any smaller than the base ELF
spec's entry structure size).
Post by Ian Jackson
However, both count values are actually 16-bit. So there is already a
limit. The extra code in elf_phdr_size is not necessary.
I'll make v2 of the patch then simply drop it.
Post by Ian Jackson
Another part of the motivation for a01b6d4 is to produce better
messages for certain malformed ELFs. There has been no assertion that
any such ELFs (ie ELFs which would fail these new checks) actually
exist or have caused trouble for anyone.
I think that it is a bad idea to add unnecessary checking to libelf.
libelf is a format parser on a security boundary. Despite attempts to
provide a safe dialect to write in, every piece of code in libelf
risks security problems.
Any crash (e.g. as a result of a signal) of course is a problem also
for a multi threaded tool stack. But beyond that I'm not sure I
understand why you say "every piece". That's perhaps partly
because ...
Post by Ian Jackson
Also, this has demonstrated that the design principles underlying the
safety of libelf are not properly understood. IIRC they were
explained in the XSA-55 00/ patch, but that's not sufficient.
... this explanation has not been recorded anywhere afaics, not
even in xsa.git.
Post by Ian Jackson
Revert all of a01b6d4.
That was already done yesterday.
Post by Ian Jackson
Delete the header count check in elf_phdr_count and replace it with a
compile-time assertion, in elf_phdr_count and in elf_shdr_count, that
sizeof the count field is <= 2. (This may need a new macro
ELF_HANDLE_FIELD_SIZEOF implemented in terms of
ELF__HANDLE_FIELD_TYPE.)
I'll wait with adding such an assertion until we've clarified the point
near the top of this reply.
Post by Ian Jackson
Add a comment near the top of libelf.h explaining the rules for
* Arithmetic on signed values is forbidden
* Use of actual pointers into the ELF is forbidden
* Division by non-constant values is forbidden
* All loops must ensure that they have a reasonably low
iteration count
* Code (including ELF format sanity checks) which is necessary
neither for safety nor for correct processing of correct files
should not be added to libelf. It is not libelf's role to
be an ELF validator.
For all these points, I'd like it to also be clarified why those are
being established. I'd appreciate if you could submit a respective
patch based on the 00/ patch you refer to above, which I
understand you still have available in your mailbox.

For this last point - "ELF validator" is certainly the goal. But I
think it is reasonable for a library to at least check the input it
makes use of. If I had meant to fully validate the ELF image,
I would e.g. have had to reject images with zero e_phnum but
non-zero other e_ph* fields. Yet we don't care about those
other fields when there are no program headers (which, as
pointed out on IRC, is useless anyway, as then there's nothing
to load; only e_shnum can sensibly be zero).

Jan
Ian Jackson
2016-12-09 11:54:51 UTC
Permalink
Post by Jan Beulich
Post by Ian Jackson
Looking at the commit message I was concerned that the phdr and shdr
counts might be excessive, and that libelf might get stuck in a loop
with an unreasonably high iteration count.
Looking at that commit message, what I'm missing first of all is an
explanation of why long loops are a problem in the first place. Do
we need to be concerned of single threaded tool stacks? I think
such a problem would better be addressed at higher layers,
switching to multi threaded designs.
libxl is (essentially) singlethreaded. It is very hard to write
correct multithreaded programs and I would be adamantly opposed to
anything that made libxl more thready inside. Also, even with
threading, a long-running domain setup would make it difficult to kill
the domain, unless we have asynchronous termination of such a thread,
which would be a nightmare to implement correctly.
Post by Jan Beulich
Post by Ian Jackson
I now see that (a) is ineffective because the image can specify its
own stride for the iteration (the header size). If it specifies a
stride of zero, the maximum count is unbounded.
Well, a stride of zero is invalid (as is any smaller than the base ELF
spec's entry structure size).
That something is invalid does not mean it cannot be specified.
Post by Jan Beulich
Post by Ian Jackson
libelf is a format parser on a security boundary. Despite attempts to
provide a safe dialect to write in, every piece of code in libelf
risks security problems.
Any crash (e.g. as a result of a signal) of course is a problem also
for a multi threaded tool stack. But beyond that I'm not sure I
understand why you say "every piece". That's perhaps partly
because ...
All code risks bugs.

The programming environment in which libelf code is written is by far
from guaranteed safe. Safety relies on programmers who write the code
following the rules. The most common kinds of mistake are rendered
harmless, but other kinds of mistake are possible.
Post by Jan Beulich
Post by Ian Jackson
Add a comment near the top of libelf.h explaining the rules for
* Arithmetic on signed values is forbidden
* Use of actual pointers into the ELF is forbidden
* Division by non-constant values is forbidden
* All loops must ensure that they have a reasonably low
iteration count
* Code (including ELF format sanity checks) which is necessary
neither for safety nor for correct processing of correct files
should not be added to libelf. It is not libelf's role to
be an ELF validator.
For all these points, I'd like it to also be clarified why those are
being established. I'd appreciate if you could submit a respective
patch based on the 00/ patch you refer to above, which I
understand you still have available in your mailbox.
I will do this.
Post by Jan Beulich
For this last point - "ELF validator" is certainly the goal. But I
think it is reasonable for a library to at least check the input it
makes use of. If I had meant to fully validate the ELF image,
I would e.g. have had to reject images with zero e_phnum but
non-zero other e_ph* fields. Yet we don't care about those
other fields when there are no program headers (which, as
pointed out on IRC, is useless anyway, as then there's nothing
to load; only e_shnum can sensibly be zero).
(I think you omitted "not", so you meant "is certainly not the goal.)

IMO libelf should do enough tests to function correctly with correct
input, and avoid being vulnerable to incorrect input. Code to perform
other tests introduces risk (of bugs, including security bugs such as
the one introduced in a01b6d4).

Such code has almost zero benefit because 1. we do not expect
ELF-generating tools to generate invalid ELFs so these checks' failure
paths will very likely never be executed anywhere, and 2. anyone
debugging such a hypothetical bad ELF will have a variety of tools
available which will do a much better job of analysing it.


Ian.
Jan Beulich
2016-12-09 13:03:04 UTC
Permalink
Jan Beulich writes ("Re: [PATCH] libelf: Fix div0 issues in
Post by Jan Beulich
Post by Ian Jackson
Looking at the commit message I was concerned that the phdr and shdr
counts might be excessive, and that libelf might get stuck in a loop
with an unreasonably high iteration count.
Looking at that commit message, what I'm missing first of all is an
explanation of why long loops are a problem in the first place. Do
we need to be concerned of single threaded tool stacks? I think
such a problem would better be addressed at higher layers,
switching to multi threaded designs.
libxl is (essentially) singlethreaded. It is very hard to write
correct multithreaded programs and I would be adamantly opposed to
anything that made libxl more thready inside. Also, even with
threading, a long-running domain setup would make it difficult to kill
the domain, unless we have asynchronous termination of such a thread,
which would be a nightmare to implement correctly.
But libxl is only a library - the question is whether xl (or whatever
containing process) may be sitting in on libelf invocation,
preventing the process(es) controlling another domain(s) from
making forward progress.

As to killing the domain - wouldn't killing the xl process doing the
creation followed by an "xl destroy" be sufficient? (As you know,
I don't know very much about how the tool stack works, but this
would seem a pretty natural pair of operations.)
Post by Jan Beulich
Post by Ian Jackson
I now see that (a) is ineffective because the image can specify its
own stride for the iteration (the header size). If it specifies a
stride of zero, the maximum count is unbounded.
Well, a stride of zero is invalid (as is any smaller than the base ELF
spec's entry structure size).
That something is invalid does not mean it cannot be specified.
And I didn't mean to imply that. What I mean to imply is that if
we guarded against too small (and hence invalid) entry size values
(which is one of the things the reverted patch does), we wouldn't
have this problem.
Post by Jan Beulich
For this last point - "ELF validator" is certainly the goal. But I
think it is reasonable for a library to at least check the input it
makes use of. If I had meant to fully validate the ELF image,
I would e.g. have had to reject images with zero e_phnum but
non-zero other e_ph* fields. Yet we don't care about those
other fields when there are no program headers (which, as
pointed out on IRC, is useless anyway, as then there's nothing
to load; only e_shnum can sensibly be zero).
(I think you omitted "not", so you meant "is certainly not the goal.)
Indeed, thanks for noticing and deducing the right thing.
IMO libelf should do enough tests to function correctly with correct
input, and avoid being vulnerable to incorrect input. Code to perform
other tests introduces risk (of bugs, including security bugs such as
the one introduced in a01b6d4).
Such code has almost zero benefit because 1. we do not expect
ELF-generating tools to generate invalid ELFs so these checks' failure
paths will very likely never be executed anywhere, and 2. anyone
debugging such a hypothetical bad ELF will have a variety of tools
available which will do a much better job of analysing it.
Well, in that case I'll have to teach myself to keep my hands off of
libelf/ as much as possible. But as you can see from the entry size
aspect further up, some extra checks may help (and might even
allow to weaken your "don't allow division by ..." criteria).

Jan
Ian Jackson
2016-12-09 15:44:42 UTC
Permalink
This will allow us to keep track of the total amount of work we are
doing. When it becomes excessive, we mark the ELF broken, and stop
processing.

This is a more robust way of preventing DoS problems by bad images
than attempting to prove, for each of the (sometimes rather deeply
nested) loops, that the total work is "reasonable". We bound the
notional work by 4x the image size (plus 1M).

Also introduce elf_strcmp_safe, which unconditionally does the work,
but increments the count so any outer loop may be aborted if
necessary.

Currently there are no callers, so no functional change.

Signed-off-by: Ian Jackson <***@eu.citrix.com>
---
xen/common/libelf/libelf-loader.c | 14 ++++++++++++++
xen/include/xen/libelf.h | 21 +++++++++++++++++++++
2 files changed, 35 insertions(+)

diff --git a/xen/common/libelf/libelf-loader.c b/xen/common/libelf/libelf-loader.c
index a72cd8a..00479af 100644
--- a/xen/common/libelf/libelf-loader.c
+++ b/xen/common/libelf/libelf-loader.c
@@ -38,6 +38,7 @@ elf_errorstatus elf_init(struct elf_binary *elf, const char *image_input, size_t
ELF_HANDLE_DECL(elf_shdr) shdr;
unsigned i, count, section, link;
uint64_t offset;
+ const uint64_t max_size_for_deacc = (1UL << 63)/ELF_MAX_ITERATION_FACTOR;

if ( !elf_is_elfbinary(image_input, size) )
{
@@ -52,6 +53,10 @@ elf_errorstatus elf_init(struct elf_binary *elf, const char *image_input, size_t
elf->class = elf_uval_3264(elf, elf->ehdr, e32.e_ident[EI_CLASS]);
elf->data = elf_uval_3264(elf, elf->ehdr, e32.e_ident[EI_DATA]);

+ elf->iteration_deaccumulator = 1024*1024 +
+ (size > max_size_for_deacc ? max_size_for_deacc : size)
+ * ELF_MAX_ITERATION_FACTOR;
+
/* Sanity check phdr. */
offset = elf_uval(elf, elf->ehdr, e_phoff) +
elf_uval(elf, elf->ehdr, e_phentsize) * elf_phdr_count(elf);
@@ -546,6 +551,15 @@ uint64_t elf_lookup_addr(struct elf_binary * elf, const char *symbol)
return value;
}

+bool elf_iter_ok_counted(struct elf_binary *elf, uint64_t maxcopysz) {
+ if (maxcopysz > elf->iteration_deaccumulator)
+ elf_mark_broken(elf, "excessive iteration - too much work to parse");
+ if (elf->broken)
+ return false;
+ elf->iteration_deaccumulator -= maxcopysz;
+ return true;
+}
+
/*
* Local variables:
* mode: C
diff --git a/xen/include/xen/libelf.h b/xen/include/xen/libelf.h
index 1b763f3..294231a 100644
--- a/xen/include/xen/libelf.h
+++ b/xen/include/xen/libelf.h
@@ -56,6 +56,8 @@ typedef void elf_log_callback(struct elf_binary*, void *caller_data,
#define ELF_MAX_STRING_LENGTH 4096
#define ELF_MAX_TOTAL_NOTE_COUNT 65536

+#define ELF_MAX_ITERATION_FACTOR 4
+
/* ------------------------------------------------------------------------ */

/* Macros for accessing the input image and output area. */
@@ -201,6 +203,9 @@ struct elf_binary {
uint64_t bsd_symtab_pstart;
uint64_t bsd_symtab_pend;

+ /* private */
+ uint64_t iteration_deaccumulator;
+
/*
* caller's other acceptable destination.
* Set by elf_set_xdest. Do not set these directly.
@@ -264,6 +269,14 @@ uint64_t elf_access_unsigned(struct elf_binary *elf, elf_ptrval ptr,

uint64_t elf_round_up(struct elf_binary *elf, uint64_t addr);

+bool elf_iter_ok_counted(struct elf_binary *elf, uint64_t count);
+ /* It is OK for count to be out by a smallish constant factor.
+ * It is OK for count to be 0, as we clamp it to 1, so we
+ * can use lengths or sizes from the image. */
+
+static inline bool elf_iter_ok(struct elf_binary *elf)
+ { return elf_iter_ok_counted(elf,1); }
+
const char *elf_strval(struct elf_binary *elf, elf_ptrval start);
/* may return NULL if the string is out of range etc. */

@@ -463,6 +476,14 @@ static inline void *elf_memset_unchecked(void *s, int c, size_t n)
* memcpy, memset and memmove to undefined MISTAKE things.
*/

+static inline int elf_strcmp_safe(struct elf_binary *elf,
+ const char *a, const char *b) {
+ elf_iter_ok_counted(elf, strlen(b));
+ return strcmp(a,b);
+}
+ /* Unlike other *_safe functions, elf_strcmp_safe is called on
+ * values already extracted from the image (eg by elf_strval),
+ * and fixed constant strings (typically, the latter is "b"). */

/* Advances past amount bytes of the current destination area. */
static inline void ELF_ADVANCE_DEST(struct elf_binary *elf, uint64_t amount)
--
2.1.4
Jan Beulich
2016-12-12 15:02:10 UTC
Permalink
Post by Ian Jackson
This will allow us to keep track of the total amount of work we are
doing. When it becomes excessive, we mark the ELF broken, and stop
processing.
This is a more robust way of preventing DoS problems by bad images
than attempting to prove, for each of the (sometimes rather deeply
nested) loops, that the total work is "reasonable". We bound the
notional work by 4x the image size (plus 1M).
The 4x has been selected arbitrarily, I suppose?
Post by Ian Jackson
--- a/xen/common/libelf/libelf-loader.c
+++ b/xen/common/libelf/libelf-loader.c
@@ -38,6 +38,7 @@ elf_errorstatus elf_init(struct elf_binary *elf, const char
*image_input, size_t
ELF_HANDLE_DECL(elf_shdr) shdr;
unsigned i, count, section, link;
uint64_t offset;
+ const uint64_t max_size_for_deacc = (1UL << 63)/ELF_MAX_ITERATION_FACTOR;
This will need adjustment for 32-bit tool stack builds - did you
perhaps mean UINT64_C(1), considering the type of the variable?

Also please add blanks around the / .
Post by Ian Jackson
@@ -546,6 +551,15 @@ uint64_t elf_lookup_addr(struct elf_binary * elf, const char *symbol)
return value;
}
+bool elf_iter_ok_counted(struct elf_binary *elf, uint64_t maxcopysz) {
+ if (maxcopysz > elf->iteration_deaccumulator)
+ elf_mark_broken(elf, "excessive iteration - too much work to parse");
+ if (elf->broken)
+ return false;
+ elf->iteration_deaccumulator -= maxcopysz;
+ return true;
+}
Hypervisor coding style here please (missing lots of blanks, and the
opening brace needs to go on its own line). I think there are more
such style problems further down.

And then - would it perhaps make sense to have most of this
function's body in #ifndef __XEN__, as there's nothing to guard
against when loading Dom0?

Jan
Ian Jackson
2016-12-12 15:23:52 UTC
Permalink
Post by Jan Beulich
Post by Ian Jackson
This is a more robust way of preventing DoS problems by bad images
than attempting to prove, for each of the (sometimes rather deeply
nested) loops, that the total work is "reasonable". We bound the
notional work by 4x the image size (plus 1M).
The 4x has been selected arbitrarily, I suppose?
Yes.
Post by Jan Beulich
Post by Ian Jackson
uint64_t offset;
+ const uint64_t max_size_for_deacc = (1UL << 63)/ELF_MAX_ITERATION_FACTOR;
This will need adjustment for 32-bit tool stack builds - did you
perhaps mean UINT64_C(1), considering the type of the variable?
Oops, yes. Although it has to be (uint64_t)1 since the tools do not
have UINT64_C.
Post by Jan Beulich
Also please add blanks around the / .
Done.
Post by Jan Beulich
Hypervisor coding style here please (missing lots of blanks, and the
opening brace needs to go on its own line). I think there are more
such style problems further down.
Sorry, I will (try to) fix the style in all the patches.
Post by Jan Beulich
And then - would it perhaps make sense to have most of this
function's body in #ifndef __XEN__, as there's nothing to guard
against when loading Dom0?
This is a good idea. If it's OK with you I'll leave the variable
initialisation etc., and simply stub out body of elf_iter_ok_counted.
If you want a more comprehensive approach I can add more #ifdefs.

Thanks,
Ian.
Jan Beulich
2016-12-12 15:15:17 UTC
Permalink
Post by Ian Jackson
+static inline bool elf_iter_ok(struct elf_binary *elf)
+ { return elf_iter_ok_counted(elf,1); }
Having seen first uses of this - why is this 1 instead of 0? Once it is,
calling elf_iter_ok_counted() here would be rather pointless, and
checking just elf_broken() here would allow the parameter to become
const.

Jan
Jan Beulich
2016-12-12 15:51:11 UTC
Permalink
Post by Ian Jackson
--- a/xen/common/libelf/libelf-loader.c
+++ b/xen/common/libelf/libelf-loader.c
@@ -38,6 +38,7 @@ elf_errorstatus elf_init(struct elf_binary *elf, const char *image_input, size_t
ELF_HANDLE_DECL(elf_shdr) shdr;
unsigned i, count, section, link;
uint64_t offset;
+ const uint64_t max_size_for_deacc = (1UL << 63)/ELF_MAX_ITERATION_FACTOR;
if ( !elf_is_elfbinary(image_input, size) )
{
@@ -52,6 +53,10 @@ elf_errorstatus elf_init(struct elf_binary *elf, const char *image_input, size_t
elf->class = elf_uval_3264(elf, elf->ehdr, e32.e_ident[EI_CLASS]);
elf->data = elf_uval_3264(elf, elf->ehdr, e32.e_ident[EI_DATA]);
+ elf->iteration_deaccumulator = 1024*1024 +
+ (size > max_size_for_deacc ? max_size_for_deacc : size)
+ * ELF_MAX_ITERATION_FACTOR;
One more question here: Is this useful at all? You're allowing
for approximately 2**63 accounted operations - how big does
an image need to be to actually break this limit? XSA-25 already
limited image sizes to 1Gb (but I do understand that the
guarding here is also against e.g. redundant loading of the
same bits through multiple program header table entries).

And how long will it take you to reach that limit (and to cause
elf->broken to be set)? With 1ns per accounted operation,
that'll be on the order of 270 years. Am I missing something
here?

Jan
Ian Jackson
2016-12-12 16:00:11 UTC
Permalink
Post by Jan Beulich
Post by Ian Jackson
+ const uint64_t max_size_for_deacc = (1UL << 63)/ELF_MAX_ITERATION_FACTOR;
...
Post by Jan Beulich
Post by Ian Jackson
+ elf->iteration_deaccumulator = 1024*1024 +
+ (size > max_size_for_deacc ? max_size_for_deacc : size)
+ * ELF_MAX_ITERATION_FACTOR;
One more question here: Is this useful at all? You're allowing
for approximately 2**63 accounted operations - how big does
an image need to be to actually break this limit? XSA-25 already
limited image sizes to 1Gb (but I do understand that the
guarding here is also against e.g. redundant loading of the
same bits through multiple program header table entries).
And how long will it take you to reach that limit (and to cause
elf->broken to be set)? With 1ns per accounted operation,
that'll be on the order of 270 years. Am I missing something
here?
I'm not sure of your point.

Mostly I allow for 4x the size of the image, plus a fixed constant of
1M operations.

The max_size_for_deacc part is necessary because otherwise the
calculation "size * ELF_MAX_ITERATION_FACTOR" might overflow. It
seems unreasonable to simply allow that overflow to occur. But if it
is causing confusion we could do that. The result would be a low
value for iteration_deaccumulator.

In practice the 1Gby image size limit will prevent this limit from
being reached, but it is distant from this check.

Ian.
Jan Beulich
2016-12-12 16:16:47 UTC
Permalink
Jan Beulich writes ("Re: [PATCH 1/8] libelf: loop safety: Introduce
Post by Jan Beulich
Post by Ian Jackson
+ const uint64_t max_size_for_deacc = (1UL << 63)/ELF_MAX_ITERATION_FACTOR;
...
Post by Jan Beulich
Post by Ian Jackson
+ elf->iteration_deaccumulator = 1024*1024 +
+ (size > max_size_for_deacc ? max_size_for_deacc : size)
+ * ELF_MAX_ITERATION_FACTOR;
One more question here: Is this useful at all? You're allowing
for approximately 2**63 accounted operations - how big does
an image need to be to actually break this limit? XSA-25 already
limited image sizes to 1Gb (but I do understand that the
guarding here is also against e.g. redundant loading of the
same bits through multiple program header table entries).
And how long will it take you to reach that limit (and to cause
elf->broken to be set)? With 1ns per accounted operation,
that'll be on the order of 270 years. Am I missing something
here?
I'm not sure of your point.
Mostly I allow for 4x the size of the image, plus a fixed constant of
1M operations.
Well, I have to confess that I've read the above as max() when
in fact it is min().
The max_size_for_deacc part is necessary because otherwise the
calculation "size * ELF_MAX_ITERATION_FACTOR" might overflow. It
seems unreasonable to simply allow that overflow to occur. But if it
is causing confusion we could do that. The result would be a low
value for iteration_deaccumulator.
Considering that overflow here will actually result in a comparably
smaller upper limit, I think this may help overall readability. But with
the above I won't insist on this in any way.

Jan
Ian Jackson
2016-12-12 16:56:43 UTC
Permalink
Post by Jan Beulich
Well, I have to confess that I've read the above as max() when
in fact it is min().
Sadly we can't use min() and max() here it seems.
Post by Jan Beulich
Post by Ian Jackson
The max_size_for_deacc part is necessary because otherwise the
calculation "size * ELF_MAX_ITERATION_FACTOR" might overflow. It
seems unreasonable to simply allow that overflow to occur. But if it
is causing confusion we could do that. The result would be a low
value for iteration_deaccumulator.
Considering that overflow here will actually result in a comparably
smaller upper limit, I think this may help overall readability. But with
the above I won't insist on this in any way.
I have replaced the limit with a comment. Now I have:

elf->iteration_deaccumulator =
1024*1024 + size * ELF_MAX_ITERATION_FACTOR;
/* overflow (from very big size, probably rejected earlier)
* would just lead to small limit, which is safe */

Ian.
Jan Beulich
2016-12-13 07:24:57 UTC
Permalink
Jan Beulich writes ("Re: [PATCH 1/8] libelf: loop safety: Introduce
Post by Jan Beulich
Well, I have to confess that I've read the above as max() when
in fact it is min().
Sadly we can't use min() and max() here it seems.
Sure, I understand that.
Post by Jan Beulich
Post by Ian Jackson
The max_size_for_deacc part is necessary because otherwise the
calculation "size * ELF_MAX_ITERATION_FACTOR" might overflow. It
seems unreasonable to simply allow that overflow to occur. But if it
is causing confusion we could do that. The result would be a low
value for iteration_deaccumulator.
Considering that overflow here will actually result in a comparably
smaller upper limit, I think this may help overall readability. But with
the above I won't insist on this in any way.
elf->iteration_deaccumulator =
1024*1024 + size * ELF_MAX_ITERATION_FACTOR;
/* overflow (from very big size, probably rejected earlier)
* would just lead to small limit, which is safe */
Thanks. May I ask that you then also use proper hypervisor
style for that comment?

Jan
Ian Jackson
2016-12-13 16:04:00 UTC
Permalink
Post by Jan Beulich
Post by Ian Jackson
elf->iteration_deaccumulator =
1024*1024 + size * ELF_MAX_ITERATION_FACTOR;
/* overflow (from very big size, probably rejected earlier)
* would just lead to small limit, which is safe */
Thanks. May I ask that you then also use proper hypervisor
style for that comment?
You mean like this ?

elf->iteration_deaccumulator =
1024*1024 + size * ELF_MAX_ITERATION_FACTOR;
/*
* overflow (from very big size, probably rejected earlier)
* would just lead to small limit, which is safe
*/

Sure, whatever. Done.

Ian.
Jan Beulich
2016-12-13 16:37:15 UTC
Permalink
Jan Beulich writes ("Re: [PATCH 1/8] libelf: loop safety: Introduce
Post by Jan Beulich
Post by Ian Jackson
elf->iteration_deaccumulator =
1024*1024 + size * ELF_MAX_ITERATION_FACTOR;
/* overflow (from very big size, probably rejected earlier)
* would just lead to small limit, which is safe */
Thanks. May I ask that you then also use proper hypervisor
style for that comment?
You mean like this ?
elf->iteration_deaccumulator =
1024*1024 + size * ELF_MAX_ITERATION_FACTOR;
/*
* overflow (from very big size, probably rejected earlier)
* would just lead to small limit, which is safe
*/
Sure, whatever. Done.
Almost: Comments are also supposed to start with an upper
case letter. But I'm fine either way.

Jan
Ian Jackson
2016-12-09 15:44:48 UTC
Permalink
Now, elf_load_image eventually calls elf_memcpy_safe, which calls
elf_iter_ok_counted.

So there is a work limit of 4x the image size. This is larger than
the previous limit of 2x the image size, but it includes a lot of
other processing too. And the purpose is to reject bad images without
a significant risk of rejecting sane ones. A 4x limit is tight
enough.

So this ad-hoc remain_allow_copy check has been entirely superseded
and can be removed.

Signed-off-by: Ian Jackson <***@eu.citrix.com>
---
xen/common/libelf/libelf-loader.c | 19 -------------------
1 file changed, 19 deletions(-)

diff --git a/xen/common/libelf/libelf-loader.c b/xen/common/libelf/libelf-loader.c
index d5e51d3..5e4671b 100644
--- a/xen/common/libelf/libelf-loader.c
+++ b/xen/common/libelf/libelf-loader.c
@@ -482,12 +482,6 @@ elf_errorstatus elf_load_binary(struct elf_binary *elf)
uint64_t paddr, offset, filesz, memsz;
unsigned i, count;
elf_ptrval dest;
- /*
- * Let bizarre ELFs write the output image up to twice; this
- * calculation is just to ensure our copying loop is no worse than
- * O(domain_size).
- */
- uint64_t remain_allow_copy = (uint64_t)elf->dest_size * 2;

count = elf_phdr_count(elf);
for ( i = 0; elf_iter_ok(elf) && i < count; i++ )
@@ -504,19 +498,6 @@ elf_errorstatus elf_load_binary(struct elf_binary *elf)
memsz = elf_uval(elf, phdr, p_memsz);
dest = elf_get_ptr(elf, paddr);

- /*
- * We need to check that the input image doesn't have us copy
- * the whole image zillions of times, as that could lead to
- * O(n^2) time behaviour and possible DoS by a malicous ELF.
- */
- if ( remain_allow_copy < memsz )
- {
- elf_mark_broken(elf, "program segments total to more"
- " than the input image size");
- break;
- }
- remain_allow_copy -= memsz;
-
elf_msg(elf,
"ELF: phdr %u at %#"ELF_PRPTRVAL" -> %#"ELF_PRPTRVAL"\n",
i, dest, (elf_ptrval)(dest + filesz));
--
2.1.4
Jan Beulich
2016-12-12 16:26:42 UTC
Permalink
Post by Ian Jackson
Now, elf_load_image eventually calls elf_memcpy_safe, which calls
elf_iter_ok_counted.
So there is a work limit of 4x the image size. This is larger than
the previous limit of 2x the image size, but it includes a lot of
other processing too. And the purpose is to reject bad images without
a significant risk of rejecting sane ones. A 4x limit is tight
enough.
So this ad-hoc remain_allow_copy check has been entirely superseded
and can be removed.
Acked-by: Jan Beulich <***@suse.com>
Ian Jackson
2016-12-09 15:44:45 UTC
Permalink
When we use elf_mem*_unsafe, we need to check that we are not doing
too much work.

Ensure that a call to elf_iter_ok_counted is near every call to
elf_mem*_unsafe.

(At one call site, just have a comment instead.)

Signed-off-by: Ian Jackson <***@eu.citrix.com>
---
xen/common/libelf/libelf-dominfo.c | 1 +
xen/common/libelf/libelf-loader.c | 2 +-
xen/common/libelf/libelf-tools.c | 6 ++++--
3 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/xen/common/libelf/libelf-dominfo.c b/xen/common/libelf/libelf-dominfo.c
index b139e32..87a47d9 100644
--- a/xen/common/libelf/libelf-dominfo.c
+++ b/xen/common/libelf/libelf-dominfo.c
@@ -498,6 +498,7 @@ elf_errorstatus elf_xen_parse(struct elf_binary *elf,
unsigned total_note_count = 0;

elf_memset_unchecked(parms, 0, sizeof(*parms));
+ elf_iter_ok_counted(elf, sizeof(*parms));
parms->virt_base = UNSET_ADDR;
parms->virt_entry = UNSET_ADDR;
parms->virt_hypercall = UNSET_ADDR;
diff --git a/xen/common/libelf/libelf-loader.c b/xen/common/libelf/libelf-loader.c
index 68c9021..d5e51d3 100644
--- a/xen/common/libelf/libelf-loader.c
+++ b/xen/common/libelf/libelf-loader.c
@@ -46,7 +46,7 @@ elf_errorstatus elf_init(struct elf_binary *elf, const char *image_input, size_t
return -1;
}

- elf_memset_unchecked(elf, 0, sizeof(*elf));
+ elf_memset_unchecked(elf, 0, sizeof(*elf)); /* loop safety: singleton */
elf->image_base = image_input;
elf->size = size;
elf->ehdr = ELF_MAKE_HANDLE(elf_ehdr, (elf_ptrval)image_input);
diff --git a/xen/common/libelf/libelf-tools.c b/xen/common/libelf/libelf-tools.c
index 56dab63..ab83150 100644
--- a/xen/common/libelf/libelf-tools.c
+++ b/xen/common/libelf/libelf-tools.c
@@ -69,7 +69,8 @@ void elf_memcpy_safe(struct elf_binary *elf, elf_ptrval dst,
elf_ptrval src, size_t size)
{
if ( elf_access_ok(elf, dst, size) &&
- elf_access_ok(elf, src, size) )
+ elf_access_ok(elf, src, size) &&
+ elf_iter_ok_counted(elf, size) )
{
/* use memmove because these checks do not prove that the
* regions don't overlap and overlapping regions grant
@@ -80,7 +81,8 @@ void elf_memcpy_safe(struct elf_binary *elf, elf_ptrval dst,

void elf_memset_safe(struct elf_binary *elf, elf_ptrval dst, int c, size_t size)
{
- if ( elf_access_ok(elf, dst, size) )
+ if ( elf_access_ok(elf, dst, size) &&
+ elf_iter_ok_counted(elf, size))
{
elf_memset_unchecked(ELF_UNSAFE_PTR(dst), c, size);
}
--
2.1.4
Jan Beulich
2016-12-12 15:19:14 UTC
Permalink
Post by Ian Jackson
@@ -80,7 +81,8 @@ void elf_memcpy_safe(struct elf_binary *elf, elf_ptrval dst,
void elf_memset_safe(struct elf_binary *elf, elf_ptrval dst, int c, size_t size)
{
- if ( elf_access_ok(elf, dst, size) )
+ if ( elf_access_ok(elf, dst, size) &&
+ elf_iter_ok_counted(elf, size))
With the style issue here fixed,
Acked-by: Jan Beulich <***@suse.com>
despite not being really happy about the added clutter.

Jan
Ian Jackson
2016-12-12 15:54:40 UTC
Permalink
Post by Jan Beulich
Post by Ian Jackson
- if ( elf_access_ok(elf, dst, size) )
+ if ( elf_access_ok(elf, dst, size) &&
+ elf_iter_ok_counted(elf, size))
With the style issue here fixed,
despite not being really happy about the added clutter.
I have added the missing space, and also moved the && to the next
line (as seems to be done in much of the hypervisor).

I hope that meets with your approval.

Ian.

_______________________________________________
Xen-devel mailing list
Jan Beulich
2016-12-12 15:58:39 UTC
Permalink
Jan Beulich writes ("Re: [PATCH 4/8] libelf: loop safety: Call
Post by Jan Beulich
Post by Ian Jackson
- if ( elf_access_ok(elf, dst, size) )
+ if ( elf_access_ok(elf, dst, size) &&
+ elf_iter_ok_counted(elf, size))
With the style issue here fixed,
despite not being really happy about the added clutter.
I have added the missing space, and also moved the && to the next
line (as seems to be done in much of the hypervisor).
I hope that meets with your approval.
Well, I don't mind either placement of the && (personally I like it
better at the front of a line, but the common model in fact is to
have it at the end).

Jan
Ian Jackson
2016-12-12 16:03:22 UTC
Permalink
Post by Jan Beulich
Post by Ian Jackson
I have added the missing space, and also moved the && to the next
line (as seems to be done in much of the hypervisor).
I hope that meets with your approval.
Well, I don't mind either placement of the && (personally I like it
better at the front of a line, but the common model in fact is to
have it at the end).
Heh. At least we agree on something here then ...

Ian.
Ian Jackson
2016-12-09 15:44:44 UTC
Permalink
In every `for' or `while' loop, either call elf_iter_ok, or explain
why it's not necessary. This is part of comprehensive defence against
out of control loops.

Signed-off-by: Ian Jackson <***@eu.citrix.com>
---
xen/common/libelf/libelf-dominfo.c | 22 +++++++++++++---------
xen/common/libelf/libelf-loader.c | 8 ++++----
xen/common/libelf/libelf-tools.c | 6 +++---
3 files changed, 20 insertions(+), 16 deletions(-)

diff --git a/xen/common/libelf/libelf-dominfo.c b/xen/common/libelf/libelf-dominfo.c
index 7f4a6a0..b139e32 100644
--- a/xen/common/libelf/libelf-dominfo.c
+++ b/xen/common/libelf/libelf-dominfo.c
@@ -43,10 +43,13 @@ elf_errorstatus elf_xen_parse_features(struct elf_binary *elf,
if ( features == NULL )
return 0;

- for ( pos = 0; features[pos] != '\0'; pos += len )
+ for ( pos = 0;
+ elf_iter_ok_counted(elf, sizeof(feature)) &&
+ features[pos] != '\0';
+ pos += len )
{
elf_memset_unchecked(feature, 0, sizeof(feature));
- for ( len = 0;; len++ )
+ for ( len = 0;; len++ ) /* can't do more than sizeof(feature) */
{
if ( len >= sizeof(feature)-1 )
break;
@@ -60,7 +63,7 @@ elf_errorstatus elf_xen_parse_features(struct elf_binary *elf,
feature[len] = features[pos + len];
}

- for ( i = 0; i < elf_xen_features; i++ )
+ for ( i = 0; elf_iter_ok(elf) && i < elf_xen_features; i++ )
{
if ( !elf_xen_feature_names[i] )
continue;
@@ -236,7 +239,7 @@ static unsigned elf_xen_parse_notes(struct elf_binary *elf,
parms->elf_note_start = start;
parms->elf_note_end = end;
for ( note = ELF_MAKE_HANDLE(elf_note, parms->elf_note_start);
- ELF_HANDLE_PTRVAL(note) < parms->elf_note_end;
+ elf_iter_ok(elf) && ELF_HANDLE_PTRVAL(note) < parms->elf_note_end;
note = elf_note_next(elf, note) )
{
#ifdef __XEN__
@@ -273,11 +276,12 @@ elf_errorstatus elf_xen_parse_guest_info(struct elf_binary *elf,

h = parms->guest_info;
#define STAR(h) (elf_access_unsigned(elf, (h), 0, 1))
- while ( STAR(h) )
+ while ( elf_iter_ok_counted(elf, sizeof(name) + sizeof(value)) &&
+ STAR(h) )
{
elf_memset_unchecked(name, 0, sizeof(name));
elf_memset_unchecked(value, 0, sizeof(value));
- for ( len = 0;; len++, h++ )
+ for ( len = 0;; len++, h++ ) /* covered by iter_ok_counted above */
{
if ( len >= sizeof(name)-1 )
break;
@@ -291,7 +295,7 @@ elf_errorstatus elf_xen_parse_guest_info(struct elf_binary *elf,
if ( STAR(h) == '=' )
{
h++;
- for ( len = 0;; len++, h++ )
+ for ( len = 0;; len++, h++ ) /* covered by iter_ok_counted */
{
if ( len >= sizeof(value)-1 )
break;
@@ -504,7 +508,7 @@ elf_errorstatus elf_xen_parse(struct elf_binary *elf,

/* Find and parse elf notes. */
count = elf_phdr_count(elf);
- for ( i = 0; i < count; i++ )
+ for ( i = 0; elf_iter_ok(elf) && i < count; i++ )
{
phdr = elf_phdr_by_index(elf, i);
if ( !elf_access_ok(elf, ELF_HANDLE_PTRVAL(phdr), 1) )
@@ -537,7 +541,7 @@ elf_errorstatus elf_xen_parse(struct elf_binary *elf,
if ( xen_elfnotes == 0 )
{
count = elf_shdr_count(elf);
- for ( i = 1; i < count; i++ )
+ for ( i = 1; elf_iter_ok(elf) && i < count; i++ )
{
shdr = elf_shdr_by_index(elf, i);
if ( !elf_access_ok(elf, ELF_HANDLE_PTRVAL(shdr), 1) )
diff --git a/xen/common/libelf/libelf-loader.c b/xen/common/libelf/libelf-loader.c
index 00479af..68c9021 100644
--- a/xen/common/libelf/libelf-loader.c
+++ b/xen/common/libelf/libelf-loader.c
@@ -85,7 +85,7 @@ elf_errorstatus elf_init(struct elf_binary *elf, const char *image_input, size_t

/* Find symbol table and symbol string table. */
count = elf_shdr_count(elf);
- for ( i = 1; i < count; i++ )
+ for ( i = 1; elf_iter_ok(elf) && i < count; i++ )
{
shdr = elf_shdr_by_index(elf, i);
if ( !elf_access_ok(elf, ELF_HANDLE_PTRVAL(shdr), 1) )
@@ -425,7 +425,7 @@ do { \
* NB: this _must_ be done one by one, and taking the bitness into account,
* so that the guest can treat this as an array of type Elf{32/64}_Shdr.
*/
- for ( i = 0; i < ELF_BSDSYM_SECTIONS; i++ )
+ for ( i = 0; elf_iter_ok(elf) && i < ELF_BSDSYM_SECTIONS; i++ )
{
rc = elf_load_image(elf, header_base + header_size + shdr_size * i,
ELF_REALPTR2PTRVAL(&header.elf_header.section[i]),
@@ -453,7 +453,7 @@ void elf_parse_binary(struct elf_binary *elf)
unsigned i, count;

count = elf_phdr_count(elf);
- for ( i = 0; i < count; i++ )
+ for ( i = 0; elf_iter_ok(elf) && i < count; i++ )
{
phdr = elf_phdr_by_index(elf, i);
if ( !elf_access_ok(elf, ELF_HANDLE_PTRVAL(phdr), 1) )
@@ -490,7 +490,7 @@ elf_errorstatus elf_load_binary(struct elf_binary *elf)
uint64_t remain_allow_copy = (uint64_t)elf->dest_size * 2;

count = elf_phdr_count(elf);
- for ( i = 0; i < count; i++ )
+ for ( i = 0; elf_iter_ok(elf) && i < count; i++ )
{
phdr = elf_phdr_by_index(elf, i);
if ( !elf_access_ok(elf, ELF_HANDLE_PTRVAL(phdr), 1) )
diff --git a/xen/common/libelf/libelf-tools.c b/xen/common/libelf/libelf-tools.c
index a9edb6a..56dab63 100644
--- a/xen/common/libelf/libelf-tools.c
+++ b/xen/common/libelf/libelf-tools.c
@@ -153,7 +153,7 @@ ELF_HANDLE_DECL(elf_shdr) elf_shdr_by_name(struct elf_binary *elf, const char *n
ELF_HANDLE_DECL(elf_shdr) shdr;
const char *sname;

- for ( i = 1; i < count; i++ )
+ for ( i = 1; elf_iter_ok(elf) && i < count; i++ )
{
shdr = elf_shdr_by_index(elf, i);
if ( !elf_access_ok(elf, ELF_HANDLE_PTRVAL(shdr), 1) )
@@ -214,7 +214,7 @@ const char *elf_strval(struct elf_binary *elf, elf_ptrval start)
if ( !elf_access_unsigned(elf, start, length, 1) )
/* ok */
return ELF_UNSAFE_PTR(start);
- if ( length >= ELF_MAX_STRING_LENGTH )
+ if ( !elf_iter_ok(elf) || length >= ELF_MAX_STRING_LENGTH )
{
elf_mark_broken(elf, "excessively long string");
return NULL;
@@ -262,7 +262,7 @@ ELF_HANDLE_DECL(elf_sym) elf_sym_by_name(struct elf_binary *elf, const char *sym
uint64_t info, name;
const char *sym_name;

- for ( ; ptr < end; ptr += elf_size(elf, sym) )
+ for ( ; elf_iter_ok(elf) && ptr < end; ptr += elf_size(elf, sym) )
{
sym = ELF_MAKE_HANDLE(elf_sym, ptr);
info = elf_uval(elf, sym, st_info);
--
2.1.4
Jan Beulich
2016-12-12 15:12:35 UTC
Permalink
Post by Ian Jackson
--- a/xen/common/libelf/libelf-dominfo.c
+++ b/xen/common/libelf/libelf-dominfo.c
@@ -43,10 +43,13 @@ elf_errorstatus elf_xen_parse_features(struct elf_binary *elf,
if ( features == NULL )
return 0;
- for ( pos = 0; features[pos] != '\0'; pos += len )
+ for ( pos = 0;
+ elf_iter_ok_counted(elf, sizeof(feature)) &&
+ features[pos] != '\0';
The two sides of the && want to align with one another.
Post by Ian Jackson
@@ -273,11 +276,12 @@ elf_errorstatus elf_xen_parse_guest_info(struct elf_binary *elf,
h = parms->guest_info;
#define STAR(h) (elf_access_unsigned(elf, (h), 0, 1))
- while ( STAR(h) )
+ while ( elf_iter_ok_counted(elf, sizeof(name) + sizeof(value)) &&
+ STAR(h) )
So you're using libelf determined sizes here rather than image
properties. Perhaps that's not a big deal, but wouldn't it be more
reasonable to simply account the whole section's size just once?
Post by Ian Jackson
@@ -490,7 +490,7 @@ elf_errorstatus elf_load_binary(struct elf_binary *elf)
uint64_t remain_allow_copy = (uint64_t)elf->dest_size * 2;
count = elf_phdr_count(elf);
- for ( i = 0; i < count; i++ )
+ for ( i = 0; elf_iter_ok(elf) && i < count; i++ )
At the example of this, but likely applicable elsewhere - what use is
this check at this point in the series? Without peeking at later patches
it is not really clear how adding this makes any difference.

Overall I'm not entirely opposed to the approach, but I find these
extra checks rather unpleasant to have.

Jan
Ian Jackson
2016-12-12 15:38:41 UTC
Permalink
Thanks for your careful review.
Post by Jan Beulich
Post by Ian Jackson
+ for ( pos = 0;
+ elf_iter_ok_counted(elf, sizeof(feature)) &&
+ features[pos] != '\0';
The two sides of the && want to align with one another.
Sure, if you like it that way.
Post by Jan Beulich
Post by Ian Jackson
@@ -273,11 +276,12 @@ elf_errorstatus elf_xen_parse_guest_info(struct elf_binary *elf,
h = parms->guest_info;
#define STAR(h) (elf_access_unsigned(elf, (h), 0, 1))
- while ( STAR(h) )
+ while ( elf_iter_ok_counted(elf, sizeof(name) + sizeof(value)) &&
+ STAR(h) )
So you're using libelf determined sizes here rather than image
properties.
I'm not sure what you mean. Each iteration of this loop contains some
calls to elf_memset_unchecked. The rules say:

* - Stack local buffer variables containing information derived from
* the image (including structs, or byte buffers) must be
* completely zeroed, using elf_memset_unchecked (and an
* accompanying elf_iter_ok_counted) on entry to the function (or
* somewhere very obviously near there).

And by elf_*_unchecked:

* Remember to call elf_iter_ok_counted nearby.

So the calls to elf_memset_unchecked, to zero name and value, imply
that there must be a call to elf_iter_ok_counted. The count parameter
should be the actual work done.

There are two loops inside this outer loop. They are textually but
not logically nested. By the rules each of these loops needs a call
to elf_iter_ok, but: since they iterate over characters for name and
value respectively, they clearly do no more work than the memsets.
Post by Jan Beulich
Perhaps that's not a big deal, but wouldn't it be more
reasonable to simply account the whole section's size just once?
You mean to call elf_iter_ok_counted on the size of the note section ?

But that would be wrong, because in principle almost each character in
the note section could cause a memset of all of name and a memset of
all of value.

Doing the iteration checks at a distance, and based on input image
properties rather than actual code flow, requires the kind of subtle
and complicated analysis I found too fragile.
Post by Jan Beulich
Post by Ian Jackson
@@ -490,7 +490,7 @@ elf_errorstatus elf_load_binary(struct elf_binary *elf)
uint64_t remain_allow_copy = (uint64_t)elf->dest_size * 2;
count = elf_phdr_count(elf);
- for ( i = 0; i < count; i++ )
+ for ( i = 0; elf_iter_ok(elf) && i < count; i++ )
At the example of this, but likely applicable elsewhere - what use is
this check at this point in the series? Without peeking at later patches
it is not really clear how adding this makes any difference.
Overall I'm not entirely opposed to the approach, but I find these
extra checks rather unpleasant to have.
I think you may need to read the big comment in patch 8. I introduce
that at the end of the series because it doesn't become true before
then.

If you like I can move it to the front of the series, and introduce it
with a "may not be true" caveat which is later removed.

Ian.
Jan Beulich
2016-12-12 15:56:53 UTC
Permalink
Post by Ian Jackson
Post by Jan Beulich
Post by Ian Jackson
@@ -273,11 +276,12 @@ elf_errorstatus elf_xen_parse_guest_info(struct elf_binary *elf,
h = parms->guest_info;
#define STAR(h) (elf_access_unsigned(elf, (h), 0, 1))
- while ( STAR(h) )
+ while ( elf_iter_ok_counted(elf, sizeof(name) + sizeof(value)) &&
+ STAR(h) )
So you're using libelf determined sizes here rather than image
properties.
I'm not sure what you mean. Each iteration of this loop contains some
* - Stack local buffer variables containing information derived from
* the image (including structs, or byte buffers) must be
* completely zeroed, using elf_memset_unchecked (and an
* accompanying elf_iter_ok_counted) on entry to the function (or
* somewhere very obviously near there).
* Remember to call elf_iter_ok_counted nearby.
So the calls to elf_memset_unchecked, to zero name and value, imply
that there must be a call to elf_iter_ok_counted. The count parameter
should be the actual work done.
Hmm, if the rules say that, I'll then have to question the rules:
Shouldn't accounting be based on what the workload the image
causes us, instead of our own overhead?
Post by Ian Jackson
Post by Jan Beulich
Perhaps that's not a big deal, but wouldn't it be more
reasonable to simply account the whole section's size just once?
You mean to call elf_iter_ok_counted on the size of the note section ?
But that would be wrong, because in principle almost each character in
the note section could cause a memset of all of name and a memset of
all of value.
Well, as per above, different ways you and I think this should work
then, as it seems. I can see where you're coming from, but I'm not
convinced this is the right model when taking a client (image)
perspective.
Post by Ian Jackson
Post by Jan Beulich
Post by Ian Jackson
@@ -490,7 +490,7 @@ elf_errorstatus elf_load_binary(struct elf_binary *elf)
uint64_t remain_allow_copy = (uint64_t)elf->dest_size * 2;
count = elf_phdr_count(elf);
- for ( i = 0; i < count; i++ )
+ for ( i = 0; elf_iter_ok(elf) && i < count; i++ )
At the example of this, but likely applicable elsewhere - what use is
this check at this point in the series? Without peeking at later patches
it is not really clear how adding this makes any difference.
Overall I'm not entirely opposed to the approach, but I find these
extra checks rather unpleasant to have.
I think you may need to read the big comment in patch 8. I introduce
that at the end of the series because it doesn't become true before
then.
Yeah, I still need to make it there (and maybe I should have looked
at that one first).

Jan
Ian Jackson
2016-12-12 16:02:49 UTC
Permalink
Post by Jan Beulich
Post by Ian Jackson
So the calls to elf_memset_unchecked, to zero name and value, imply
that there must be a call to elf_iter_ok_counted. The count parameter
should be the actual work done.
Shouldn't accounting be based on what the workload the image
causes us, instead of our own overhead?
The purpose of the accounting is to prevent the image from causing us
to do lots of work. The work calculation should be based on the
actual algorithm, not on some hypothetical other algorithm that might
be more efficient.

Otherwise if our algorithm is inefficient in some surprising way, when
faced with certain unusual images, that would be a DOS vulnerability.

I think it is easier to write these checks, in terms of the actual
work done, than attempt to construct a proof that the algorithm always
only does a reasonable amount of work.

Ian.
Ian Jackson
2016-12-09 15:44:49 UTC
Permalink
Signed-off-by: Ian Jackson <***@eu.citrix.com>
---
xen/include/xen/libelf.h | 92 ++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 92 insertions(+)

diff --git a/xen/include/xen/libelf.h b/xen/include/xen/libelf.h
index 6436bd7..8b75242 100644
--- a/xen/include/xen/libelf.h
+++ b/xen/include/xen/libelf.h
@@ -60,6 +60,96 @@ typedef void elf_log_callback(struct elf_binary*, void *caller_data,

/* ------------------------------------------------------------------------ */

+/*
+ * DESIGN PRINCIPLES FOR THE SAFETY OF LIBELF
+ *
+ * libelf is a complex piece of code on a security boundary: when
+ * built as part of the tools, it parses guest kernels and loads them
+ * into guest memory. Bugs in libelf can become privilege escalation
+ * or denial of service bugs in the toolstack.
+ *
+ * We try to reduce the risk of such bugs by writing the actual format
+ * parsing in a mostly-safe subset of C. To avoid nonlocal exits or
+ * the need for explicit error-checking code, we make all references
+ * into the input image, or into guest memory, via an inherently safe
+ * wrapper system.
+ *
+ * This means that it is safe to simply honour the instructions from
+ * the image, even if they are nonsense. If the image implies wild
+ * pointer accesses, these will be harmlessly defused; a note will be
+ * made that things are broken; and processing can safely continue
+ * despite some of the operations having not been done. Eventually
+ * the error will be reported.
+ *
+ *
+ * To preserve these safety properties, there are some rules that
+ * programmers editing libelf need to follow:
+ *
+ * - Any loop needs to be accompanied by calls to elf_iter_ok (or
+ * elf_iter_ok_counted).
+ *
+ * Rationale: the image must not be able to cause libelf to do
+ * unbounded work (ie, get stuck in a loop).
+ *
+ * - The input image and output area must be accessed only via the
+ * safe pointer access system. Real pointers into the input or
+ * output may not be even *calculated*.
+ *
+ * Rationale: calculating wild pointers is undefined behaviour;
+ * if the compiler sees that you might be calculating wild
+ * pointers, it may remove important checks!
+ *
+ * - Stack local buffer variables containing information derived from
+ * the image (including structs, or byte buffers) must be
+ * completely zeroed, using elf_memset_unchecked (and an
+ * accompanying elf_iter_ok_counted) on entry to the function (or
+ * somewhere very obviously near there).
+ *
+ * Rationale: This avoids uninitialised stack data being used
+ * as input to any of the loader.
+ *
+ * - All integer variables should be unsigned.
+ *
+ * Rationale: this avoids signed integer overflow (which has
+ * undefined behaviour in C, and if spotted by the compiler can
+ * cause it to generate bad code).
+ *
+ * - Arithmetic operations other than + - * should be avoided; in
+ * particular, division (/ or %) by non-constant values should be
+ * avoided. If it cannot be avoided, the divisor must be checked
+ * for zero.
+ *
+ * Rationale: We must avoid division-by-zero (or other overflow
+ * traps).
+ *
+ * - If it is desirable to breach these rules, there should be a
+ * comment explaining why this is OK.
+ *
+ * Even so, this is a fairly high-risk environment, so:
+ *
+ * - Do not add code which is not necessary for libelf to function
+ * with correct input, or to avoid being vulnerable to incorrect
+ * input. Do not add additional functionally-unnecessary checks
+ * for diagnosing problems with the image, or validating sanity of
+ * the input ELF.
+ *
+ * Rationale: Redundant checks have almost zero benefit because
+ * 1. we do not expect ELF-generating tools to generate invalid
+ * ELFs so these checks' failure paths will very likely never
+ * be executed anywhere, and 2. anyone debugging such a
+ * hypothetical bad ELF will have a variety of tools available
+ * which will do a much better job of analysing it.
+ *
+ * - However, it is OK to have checks code which provide duplicate
+ * defence against certain hostile images, if it is not otherwise
+ * obvious how libelf would be defended against such images.
+ *
+ * Rationale: Redundant checks where the situation would
+ * otherwise not be quite clear mean that the safety of the
+ * code is easy to see throughout; so that any unsafe code
+ * would be more obvious.
+ */
+
/* Macros for accessing the input image and output area. */

/*
@@ -475,6 +565,8 @@ static inline void *elf_memset_unchecked(void *s, int c, size_t n)
* pointers. These are just like the real functions.
* We provide these so that in libelf-private.h we can #define
* memcpy, memset and memmove to undefined MISTAKE things.
+ *
+ * Remember to call elf_iter_ok_counted nearby.
*/

static inline int elf_strcmp_safe(struct elf_binary *elf,
--
2.1.4
Jan Beulich
2016-12-15 16:43:45 UTC
Permalink
Post by Ian Jackson
+/*
+ * DESIGN PRINCIPLES FOR THE SAFETY OF LIBELF
+ *
+ * libelf is a complex piece of code on a security boundary: when
+ * built as part of the tools, it parses guest kernels and loads them
+ * into guest memory. Bugs in libelf can become privilege escalation
+ * or denial of service bugs in the toolstack.
+ *
+ * We try to reduce the risk of such bugs by writing the actual format
+ * parsing in a mostly-safe subset of C. To avoid nonlocal exits or
+ * the need for explicit error-checking code, we make all references
+ * into the input image, or into guest memory, via an inherently safe
+ * wrapper system.
+ *
+ * This means that it is safe to simply honour the instructions from
+ * the image, even if they are nonsense. If the image implies wild
+ * pointer accesses, these will be harmlessly defused; a note will be
+ * made that things are broken; and processing can safely continue
+ * despite some of the operations having not been done. Eventually
+ * the error will be reported.
+ *
+ *
+ * To preserve these safety properties, there are some rules that
+ *
+ * - Any loop needs to be accompanied by calls to elf_iter_ok (or
+ * elf_iter_ok_counted).
+ *
+ * Rationale: the image must not be able to cause libelf to do
+ * unbounded work (ie, get stuck in a loop).
As expressed before, I'm not convinced library code should be
concerned about caller restrictions.
Post by Ian Jackson
+ * - The input image and output area must be accessed only via the
+ * safe pointer access system. Real pointers into the input or
+ * output may not be even *calculated*.
+ *
+ * Rationale: calculating wild pointers is undefined behaviour;
+ * if the compiler sees that you might be calculating wild
+ * pointers, it may remove important checks!
+ *
+ * - Stack local buffer variables containing information derived from
+ * the image (including structs, or byte buffers) must be
+ * completely zeroed, using elf_memset_unchecked (and an
+ * accompanying elf_iter_ok_counted) on entry to the function (or
+ * somewhere very obviously near there).
+ *
+ * Rationale: This avoids uninitialised stack data being used
+ * as input to any of the loader.
What distinguishes a "buffer variable" from a plain variable? I.e.
where is the boundary here as to which ones need zeroing? Also,
I don't think zeroing is needed when a variable gets fully written
over (like in a structure assignment). Do you perhaps want to
limit the above to "directly derived from the image"?
Post by Ian Jackson
+ * - All integer variables should be unsigned.
+ *
+ * Rationale: this avoids signed integer overflow (which has
+ * undefined behaviour in C, and if spotted by the compiler can
+ * cause it to generate bad code).
There are certainly cases where one explicitly needs negative
values, and hence signed integer types. Therefore I don't think
we can outright exclude this. Perhaps limit by "... variables with
non-negative value range ..."?
Post by Ian Jackson
+ * - Arithmetic operations other than + - * should be avoided; in
+ * particular, division (/ or %) by non-constant values should be
+ * avoided. If it cannot be avoided, the divisor must be checked
+ * for zero.
+ *
+ * Rationale: We must avoid division-by-zero (or other overflow
+ * traps).
+ *
+ * - If it is desirable to breach these rules, there should be a
+ * comment explaining why this is OK.
+ *
+ *
+ * - Do not add code which is not necessary for libelf to function
+ * with correct input, or to avoid being vulnerable to incorrect
+ * input. Do not add additional functionally-unnecessary checks
+ * for diagnosing problems with the image, or validating sanity of
+ * the input ELF.
+ *
+ * Rationale: Redundant checks have almost zero benefit because
+ * 1. we do not expect ELF-generating tools to generate invalid
+ * ELFs so these checks' failure paths will very likely never
+ * be executed anywhere, and 2. anyone debugging such a
+ * hypothetical bad ELF will have a variety of tools available
+ * which will do a much better job of analysing it.
While I can understand your reasoning, I continue to think that
libelf in a ELF loader, and hence should check certain things. The
best example I currently know of are e_[ps]hentsize, which the
code uses without checking the lower valid bound.

Overall I'm certainly not meaning to veto any of this, but I think it
would be better for some other REST maintainer (agreeing with
your way of thinking) to ack this.

Jan
George Dunlap
2016-12-16 04:28:00 UTC
Permalink
Post by Jan Beulich
Post by Ian Jackson
+ * - Any loop needs to be accompanied by calls to elf_iter_ok (or
+ * elf_iter_ok_counted).
+ *
+ * Rationale: the image must not be able to cause libelf to do
+ * unbounded work (ie, get stuck in a loop).
As expressed before, I'm not convinced library code should be
concerned about caller restrictions.
People designing toolstacks that call this function are likely to be thinking about domains and things, not, “What happens if I get a rogue elf image that causes this function to run forever?” I think if we can prevent libelf-source DoS bugs in all toolstacks that rely on libxl, then it makes sense to do so.

-George
Ian Jackson
2016-12-16 11:33:52 UTC
Permalink
Post by George Dunlap
Post by Jan Beulich
As expressed before, I'm not convinced library code should be
concerned about caller restrictions.
I'm not sure what you mean by "caller restrictions". This is a rule
that applies inside libelf. Can you explain ?
Post by George Dunlap
People designing toolstacks that call this function are likely to be thinking about domains and things, not, “What happens if I get a rogue elf image that causes this function to run forever?” I think if we can prevent libelf-source DoS bugs in all toolstacks that rely on libxl, then it makes sense to do so.
I think in fact the only caller of libelf is libxc. I doubt we have
out-of-tree callers, but I could be wrong of course.

But that doesn't change the underlying point, which I agree with:
callers of any library should be given information on how to use it
safely.

Our libelf does not have a 100% opaque interface for its callers. For
example, the state struct is transparent, and the multiple entry
points are not entirely clearly set out.

If it did have a more formally defined and opaque interface, then much
of what is currently in libelf.h (including this comment) would be in
libelf-private.h instead.

Thanks,
Ian.
Jan Beulich
2016-12-16 11:58:03 UTC
Permalink
George Dunlap writes ("Re: [PATCH 8/8] libelf: safety: Document safety
Post by Jan Beulich
As expressed before, I'm not convinced library code should be
concerned about caller restrictions.
I'm not sure what you mean by "caller restrictions". This is a rule
that applies inside libelf. Can you explain ?
This goes back to the single threaded discussion we've had. As a
library it shouldn't be concerned how long it takes for it to complete
its job, as long as it follows what the EFL image says (and that
image is either valid, or errors get surfaced for any invalid elements).

Jan
Ian Jackson
2016-12-16 11:43:06 UTC
Permalink
Post by Jan Beulich
Post by Ian Jackson
+ * - Stack local buffer variables containing information derived from
+ * the image (including structs, or byte buffers) must be
+ * completely zeroed, using elf_memset_unchecked (and an
+ * accompanying elf_iter_ok_counted) on entry to the function (or
+ * somewhere very obviously near there).
+ *
+ * Rationale: This avoids uninitialised stack data being used
+ * as input to any of the loader.
What distinguishes a "buffer variable" from a plain variable? I.e.
where is the boundary here as to which ones need zeroing?
The hazard is an undiscovered uninitialised variable. "Undiscovered"
meaning static analysis tools won't find it (eg Coverity will find
many if not most normal uninitialised variables).

This is the biggest risk for variables which can be partially
initialised: ie, structs and arrays. It is more difficult to reason
about variables which can be partially initialised.
Post by Jan Beulich
Also,
I don't think zeroing is needed when a variable gets fully written
over (like in a structure assignment). Do you perhaps want to
limit the above to "directly derived from the image"?
Structure assignment does not necessarily write over the whole
variable. It may leave the padding un-overwritten.
Post by Jan Beulich
Post by Ian Jackson
+ * - All integer variables should be unsigned.
+ *
+ * Rationale: this avoids signed integer overflow (which has
+ * undefined behaviour in C, and if spotted by the compiler can
+ * cause it to generate bad code).
There are certainly cases where one explicitly needs negative
values, and hence signed integer types. Therefore I don't think
we can outright exclude this. Perhaps limit by "... variables with
non-negative value range ..."?
There are AFAICT the following signed variables in libelf: some error
codes; the c parameter to elf_memset_safe; the `nr' parameters to
elf_xen_feature_set/get.

I think perhaps the exceptions should be mentioned. The error codes
are unfortunate but they are safe and too hard to change, and should
get a comment. The c and nr parameters should be made unsigned. The
open-coded ints for some function return values should be changed to
elf_errorstatus.

I will update the series to improve this.

Anyway, it is not necessary to deal with actually negative
quantities. If it should become necessary in the future then:

+ * - If it is desirable to breach these rules, there should be a
+ * comment explaining why this is OK.
Post by Jan Beulich
Post by Ian Jackson
+ * - Do not add code which is not necessary for libelf to function
+ * with correct input, or to avoid being vulnerable to incorrect
+ * input. Do not add additional functionally-unnecessary checks
+ * for diagnosing problems with the image, or validating sanity of
+ * the input ELF.
...
Post by Jan Beulich
While I can understand your reasoning, I continue to think that
libelf in a ELF loader, and hence should check certain things. The
best example I currently know of are e_[ps]hentsize, which the
code uses without checking the lower valid bound.
Post by Ian Jackson
+ * Rationale: Redundant checks have almost zero benefit because
+ * 1. we do not expect ELF-generating tools to generate invalid
+ * ELFs so these checks' failure paths will very likely never
+ * be executed anywhere, and 2. anyone debugging such a
+ * hypothetical bad ELF will have a variety of tools available
+ * which will do a much better job of analysing it.
I don't find "libelf in a ELF loader" a convincing counterargument.

In practical terms, not checking e_[ps]hentsize means that
hypothetical broken images with too-small e_[ps]hentsize will result
in either a confusing error reported later by some other bit of libelf
(for example, due to an out-of-bounds access setting the `broken'
flag), or a badly-constructed guest. This is IMO a quite tolerable
consequence. Following a confusing error message the afflicted user
is very likely to do something sensible, like feeding their image to a
disassembler, which would immediately diagnose the problem. Even
if the result is a badly-constructed guest, the symptoms are very
likely to induce an appropriate debugging response, and not likely to
cause great inconvenience (compared to a specific diagnosis).

Especially, since (and AFIACT you are not disputing this) we do not
expect that such a check's failure path would ever be executed
anywhere. The expected benefit of the extra check is negligible.

The cost of the extra check is the risk of making mistakes, resulting
in security vulnerabilities. Security vulnerabilties have a high
cost. So even if the probability of making such a mistake is fairly
low, the expected cost of the extra checks is definitely
non-negligible.
Post by Jan Beulich
Overall I'm certainly not meaning to veto any of this, but I think it
would be better for some other REST maintainer (agreeing with
your way of thinking) to ack this.
Thanks anyway for your careful review and attention to detail.

George, since you seem to be reading this thread: what do you think
about this disagreement about the right direction to go ? I think
that the fact that the other approach resulted in us committing a
vulnerability to staging is a strong indication that we need better
defence against (inevitable) human error.

Thanks,
Ian.
Jan Beulich
2016-12-16 12:31:38 UTC
Permalink
Jan Beulich writes ("Re: [PATCH 8/8] libelf: safety: Document safety
Post by Jan Beulich
Post by Ian Jackson
+ * - Stack local buffer variables containing information derived from
+ * the image (including structs, or byte buffers) must be
+ * completely zeroed, using elf_memset_unchecked (and an
+ * accompanying elf_iter_ok_counted) on entry to the function (or
+ * somewhere very obviously near there).
+ *
+ * Rationale: This avoids uninitialised stack data being used
+ * as input to any of the loader.
What distinguishes a "buffer variable" from a plain variable? I.e.
where is the boundary here as to which ones need zeroing?
The hazard is an undiscovered uninitialised variable. "Undiscovered"
meaning static analysis tools won't find it (eg Coverity will find
many if not most normal uninitialised variables).
This is the biggest risk for variables which can be partially
initialised: ie, structs and arrays. It is more difficult to reason
about variables which can be partially initialised.
I guess you meant "can't"? In any event the wording may want
some clarification then.
Post by Jan Beulich
Also,
I don't think zeroing is needed when a variable gets fully written
over (like in a structure assignment). Do you perhaps want to
limit the above to "directly derived from the image"?
Structure assignment does not necessarily write over the whole
variable. It may leave the padding un-overwritten.
Oh, true.
Post by Jan Beulich
Post by Ian Jackson
+ * - Do not add code which is not necessary for libelf to function
+ * with correct input, or to avoid being vulnerable to incorrect
+ * input. Do not add additional functionally-unnecessary checks
+ * for diagnosing problems with the image, or validating sanity of
+ * the input ELF.
...
Post by Jan Beulich
While I can understand your reasoning, I continue to think that
libelf in a ELF loader, and hence should check certain things. The
best example I currently know of are e_[ps]hentsize, which the
code uses without checking the lower valid bound.
Post by Ian Jackson
+ * Rationale: Redundant checks have almost zero benefit because
+ * 1. we do not expect ELF-generating tools to generate invalid
+ * ELFs so these checks' failure paths will very likely never
+ * be executed anywhere, and 2. anyone debugging such a
+ * hypothetical bad ELF will have a variety of tools available
+ * which will do a much better job of analysing it.
Well, because as said - I can understand it, given the perspective
you take.

Jan

Ian Jackson
2016-12-09 15:44:43 UTC
Permalink
Not used yet, so no functional change. We will need this in a moment.

Signed-off-by: Ian Jackson <***@eu.citrix.com>
---
xen/common/libelf/libelf-dominfo.c | 5 +++--
xen/include/xen/libelf.h | 3 ++-
2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/xen/common/libelf/libelf-dominfo.c b/xen/common/libelf/libelf-dominfo.c
index a52900c..7f4a6a0 100644
--- a/xen/common/libelf/libelf-dominfo.c
+++ b/xen/common/libelf/libelf-dominfo.c
@@ -32,7 +32,8 @@ static const char *const elf_xen_feature_names[] = {
static const unsigned elf_xen_features =
sizeof(elf_xen_feature_names) / sizeof(elf_xen_feature_names[0]);

-elf_errorstatus elf_xen_parse_features(const char *features,
+elf_errorstatus elf_xen_parse_features(struct elf_binary *elf,
+ const char *features,
uint32_t *supported,
uint32_t *required)
{
@@ -202,7 +203,7 @@ elf_errorstatus elf_xen_parse_note(struct elf_binary *elf,
break;

case XEN_ELFNOTE_FEATURES:
- if ( elf_xen_parse_features(str, parms->f_supported,
+ if ( elf_xen_parse_features(elf, str, parms->f_supported,
parms->f_required) )
return -1;
break;
diff --git a/xen/include/xen/libelf.h b/xen/include/xen/libelf.h
index 294231a..6436bd7 100644
--- a/xen/include/xen/libelf.h
+++ b/xen/include/xen/libelf.h
@@ -452,7 +452,8 @@ static inline int elf_xen_feature_get(int nr, uint32_t * addr)
return !!(addr[nr >> 5] & (1 << (nr & 31)));
}

-int elf_xen_parse_features(const char *features,
+int elf_xen_parse_features(struct elf_binary *elf,
+ const char *features,
uint32_t *supported,
uint32_t *required);
int elf_xen_parse_note(struct elf_binary *elf,
--
2.1.4
Jan Beulich
2016-12-12 15:03:01 UTC
Permalink
Post by Ian Jackson
Not used yet, so no functional change. We will need this in a moment.
Acked-by: Jan Beulich <***@suse.com>
Ian Jackson
2016-12-09 15:44:47 UTC
Permalink
All the loops which might go out of control, due to excessive shdrs,
have been decorated with elf_iter_ok. So there is no need for this
explicit (and rather crude) check.

(Anyway, the count was a 16-bit field, so the check was redundant.)

Signed-off-by: Ian Jackson <***@eu.citrix.com>
---
xen/common/libelf/libelf-tools.c | 12 +-----------
1 file changed, 1 insertion(+), 11 deletions(-)

diff --git a/xen/common/libelf/libelf-tools.c b/xen/common/libelf/libelf-tools.c
index 7fa5963..b799b56 100644
--- a/xen/common/libelf/libelf-tools.c
+++ b/xen/common/libelf/libelf-tools.c
@@ -131,17 +131,7 @@ uint64_t elf_round_up(struct elf_binary *elf, uint64_t addr)

unsigned elf_shdr_count(struct elf_binary *elf)
{
- unsigned count = elf_uval(elf, elf->ehdr, e_shnum);
- uint64_t max = elf->size / sizeof(Elf32_Shdr);
-
- if ( max > UINT_MAX )
- max = UINT_MAX;
- if ( count > max )
- {
- elf_mark_broken(elf, "far too many section headers");
- count = max;
- }
- return count;
+ return elf_uval(elf, elf->ehdr, e_shnum);
}

unsigned elf_phdr_count(struct elf_binary *elf)
--
2.1.4
Jan Beulich
2016-12-12 15:41:40 UTC
Permalink
Post by Ian Jackson
All the loops which might go out of control, due to excessive shdrs,
have been decorated with elf_iter_ok. So there is no need for this
explicit (and rather crude) check.
(Anyway, the count was a 16-bit field, so the check was redundant.)
Reviewed-by: Jan Beulich <***@suse.com>
Ian Jackson
2016-12-09 15:44:46 UTC
Permalink
strcmp can do singificant work, and is found in some inner loops where
we search for the meaning of things we find in the image. We need to
avoid doing too much work.

So replace all calls to strcmp with elf_strcmp_safe.

Signed-off-by: Ian Jackson <***@eu.citrix.com>
---
xen/common/libelf/libelf-dominfo.c | 37 +++++++++++++++++++------------------
xen/common/libelf/libelf-private.h | 7 ++++---
xen/common/libelf/libelf-tools.c | 4 ++--
3 files changed, 25 insertions(+), 23 deletions(-)

diff --git a/xen/common/libelf/libelf-dominfo.c b/xen/common/libelf/libelf-dominfo.c
index 87a47d9..b037d10 100644
--- a/xen/common/libelf/libelf-dominfo.c
+++ b/xen/common/libelf/libelf-dominfo.c
@@ -70,7 +70,8 @@ elf_errorstatus elf_xen_parse_features(struct elf_binary *elf,
if ( feature[0] == '!' )
{
/* required */
- if ( !strcmp(feature + 1, elf_xen_feature_names[i]) )
+ if ( !elf_strcmp_safe(elf, feature + 1,
+ elf_xen_feature_names[i]) )
{
elf_xen_feature_set(i, supported);
if ( required )
@@ -81,7 +82,7 @@ elf_errorstatus elf_xen_parse_features(struct elf_binary *elf,
else
{
/* supported */
- if ( !strcmp(feature, elf_xen_feature_names[i]) )
+ if ( !elf_strcmp_safe(elf, feature, elf_xen_feature_names[i]) )
{
elf_xen_feature_set(i, supported);
break;
@@ -173,13 +174,13 @@ elf_errorstatus elf_xen_parse_note(struct elf_binary *elf,
safe_strcpy(parms->xen_ver, str);
break;
case XEN_ELFNOTE_PAE_MODE:
- if ( !strcmp(str, "yes") )
+ if ( !elf_strcmp_safe(elf, str, "yes") )
parms->pae = XEN_PAE_EXTCR3;
if ( strstr(str, "bimodal") )
parms->pae = XEN_PAE_BIMODAL;
break;
case XEN_ELFNOTE_BSD_SYMTAB:
- if ( !strcmp(str, "yes") )
+ if ( !elf_strcmp_safe(elf, str, "yes") )
parms->bsd_symtab = 1;
break;

@@ -255,7 +256,7 @@ static unsigned elf_xen_parse_notes(struct elf_binary *elf,
note_name = elf_note_name(elf, note);
if ( note_name == NULL )
continue;
- if ( strcmp(note_name, "Xen") )
+ if ( elf_strcmp_safe(elf, note_name, "Xen") )
continue;
if ( elf_xen_parse_note(elf, parms, note) )
return ELF_NOTE_INVALID;
@@ -315,38 +316,38 @@ elf_errorstatus elf_xen_parse_guest_info(struct elf_binary *elf,
elf_msg(elf, "ELF: %s=\"%s\"\n", name, value);

/* strings */
- if ( !strcmp(name, "LOADER") )
+ if ( !elf_strcmp_safe(elf, name, "LOADER") )
safe_strcpy(parms->loader, value);
- if ( !strcmp(name, "GUEST_OS") )
+ if ( !elf_strcmp_safe(elf, name, "GUEST_OS") )
safe_strcpy(parms->guest_os, value);
- if ( !strcmp(name, "GUEST_VER") )
+ if ( !elf_strcmp_safe(elf, name, "GUEST_VER") )
safe_strcpy(parms->guest_ver, value);
- if ( !strcmp(name, "XEN_VER") )
+ if ( !elf_strcmp_safe(elf, name, "XEN_VER") )
safe_strcpy(parms->xen_ver, value);
- if ( !strcmp(name, "PAE") )
+ if ( !elf_strcmp_safe(elf, name, "PAE") )
{
- if ( !strcmp(value, "yes[extended-cr3]") )
+ if ( !elf_strcmp_safe(elf, value, "yes[extended-cr3]") )
parms->pae = XEN_PAE_EXTCR3;
else if ( !strncmp(value, "yes", 3) )
parms->pae = XEN_PAE_YES;
}
- if ( !strcmp(name, "BSD_SYMTAB") )
+ if ( !elf_strcmp_safe(elf, name, "BSD_SYMTAB") )
parms->bsd_symtab = 1;

/* longs */
- if ( !strcmp(name, "VIRT_BASE") )
+ if ( !elf_strcmp_safe(elf, name, "VIRT_BASE") )
parms->virt_base = strtoull(value, NULL, 0);
- if ( !strcmp(name, "VIRT_ENTRY") )
+ if ( !elf_strcmp_safe(elf, name, "VIRT_ENTRY") )
parms->virt_entry = strtoull(value, NULL, 0);
- if ( !strcmp(name, "ELF_PADDR_OFFSET") )
+ if ( !elf_strcmp_safe(elf, name, "ELF_PADDR_OFFSET") )
parms->elf_paddr_offset = strtoull(value, NULL, 0);
- if ( !strcmp(name, "HYPERCALL_PAGE") )
+ if ( !elf_strcmp_safe(elf, name, "HYPERCALL_PAGE") )
parms->virt_hypercall = (strtoull(value, NULL, 0) << 12) +
parms->virt_base;

/* other */
- if ( !strcmp(name, "FEATURES") )
- if ( elf_xen_parse_features(value, parms->f_supported,
+ if ( !elf_strcmp_safe(elf, name, "FEATURES") )
+ if ( elf_xen_parse_features(elf, value, parms->f_supported,
parms->f_required) )
return -1;
}
diff --git a/xen/common/libelf/libelf-private.h b/xen/common/libelf/libelf-private.h
index 388c3da..082c572 100644
--- a/xen/common/libelf/libelf-private.h
+++ b/xen/common/libelf/libelf-private.h
@@ -98,9 +98,10 @@ do { strncpy((d),(s),sizeof((d))-1); \
#define memset MISTAKE_unspecified_memset
#define memmove MISTAKE_unspecified_memmove
#define strcpy MISTAKE_unspecified_strcpy
- /* This prevents libelf from using these undecorated versions
- * of memcpy, memset, memmove and strcpy. Every call site
- * must either use elf_mem*_unchecked, or elf_mem*_safe. */
+#define strcmp MISTAKE_unspecified_strcmp
+ /* This prevents libelf from using these undecorated versions
+ * of memcpy, memset, memmove, strcpy and strcmp. Every call site
+ * must either use elf_mem*_unchecked, or elf_*_safe. */

#endif /* __LIBELF_PRIVATE_H__ */

diff --git a/xen/common/libelf/libelf-tools.c b/xen/common/libelf/libelf-tools.c
index ab83150..7fa5963 100644
--- a/xen/common/libelf/libelf-tools.c
+++ b/xen/common/libelf/libelf-tools.c
@@ -162,7 +162,7 @@ ELF_HANDLE_DECL(elf_shdr) elf_shdr_by_name(struct elf_binary *elf, const char *n
/* input has an insane section header count field */
break;
sname = elf_section_name(elf, shdr);
- if ( sname && !strcmp(sname, name) )
+ if ( sname && !elf_strcmp_safe(elf, sname, name) )
return shdr;
}
return ELF_INVALID_HANDLE(elf_shdr);
@@ -274,7 +274,7 @@ ELF_HANDLE_DECL(elf_sym) elf_sym_by_name(struct elf_binary *elf, const char *sym
sym_name = elf_strval(elf, elf->sym_strtab + name);
if ( sym_name == NULL ) /* out of range, oops */
return ELF_INVALID_HANDLE(elf_sym);
- if ( strcmp(sym_name, symbol) )
+ if ( elf_strcmp_safe(elf, sym_name, symbol) )
continue;
return sym;
}
--
2.1.4
Jan Beulich
2016-12-12 15:22:49 UTC
Permalink
Post by Ian Jackson
/* other */
- if ( !strcmp(name, "FEATURES") )
- if ( elf_xen_parse_features(value, parms->f_supported,
+ if ( !elf_strcmp_safe(elf, name, "FEATURES") )
+ if ( elf_xen_parse_features(elf, value, parms->f_supported,
parms->f_required) )
return -1;
There must be some patch split problem here - this patch does not
alter the parameters of elf_xen_parse_features(), so the argument
list can't change here.

And one of the then (at least) two patches touching this code could
then take the opportunity and fold the two if()s into one.

Jan
Ian Jackson
2016-12-12 15:44:08 UTC
Permalink
Post by Jan Beulich
Post by Ian Jackson
/* other */
- if ( !strcmp(name, "FEATURES") )
- if ( elf_xen_parse_features(value, parms->f_supported,
+ if ( !elf_strcmp_safe(elf, name, "FEATURES") )
+ if ( elf_xen_parse_features(elf, value, parms->f_supported,
parms->f_required) )
return -1;
There must be some patch split problem here - this patch does not
alter the parameters of elf_xen_parse_features(), so the argument
list can't change here.
Yes. That part of this hunk should be in
libelf: loop safety: Pass `elf' to elf_xen_parse_features
I thought I had done a compile test of that individual patch but
I must have messed that up somehow. I will move that change.
Post by Jan Beulich
And one of the then (at least) two patches touching this code could
then take the opportunity and fold the two if()s into one.
Looking at the surrounding code, I actually prefer it the way it is.
It makes it more like the other similar test/handle pairs.

Thanks,
Ian.
Ian Jackson
2016-12-08 14:48:17 UTC
Permalink
Post by Jan Beulich
Post by Andrew Cooper
Spotted by Coverity.
And wrongly so, imo.
...
Post by Jan Beulich
Post by Andrew Cooper
@@ -148,11 +154,17 @@ unsigned elf_shdr_count(struct elf_binary *elf)
unsigned elf_phdr_count(struct elf_binary *elf)
{
unsigned count = elf_uval(elf, elf->ehdr, e_phnum);
+ unsigned entsize = elf_uval(elf, elf->ehdr, e_phentsize);
...
Post by Jan Beulich
... this would end up being dead code, due to the checks the same
patch you refer to introduced in elf_init().
I think I would prefer code that is obviously correct from local
inspection. There is no performance implication here.

Maybe what we want is elf_divide() which implments anything/0 as 0.

Ian.
Loading...