Re: [PATCH] tools/xenstored: Harden corrupt()

[Julien Grall <julien@xxxxxxx>]

Hi Juergen,

On 23/06/2022 12:28, Juergen Gross wrote:
> On 23.06.22 13:24, Julien Grall wrote:
> > From: Julien Grall <jgrall@xxxxxxxxxx>
> >
> > At the moment, corrupt() is neither checking for allocation failure
> > nor freeing the allocated memory.
> >
> > Harden the code by printing ENOMEM if the allocation failed and
> > free 'str' after the last use.
> >
> > This is not considered to be a security issue because corrupt() should
> > only be called when Xenstored thinks the database is corrupted. Note
> > that the trigger (i.e. a guest reliably provoking the call) would be
> > a security issue.
> >
> > Fixes: 06d17943f0cd ("Added a basic integrity checker, and some basic 
> > ability to recover from store")
> > Signed-off-by: Julien Grall <jgrall@xxxxxxxxxx>
> > ---
> >   tools/xenstore/xenstored_core.c | 6 +++++-
> >   1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/tools/xenstore/xenstored_core.c 
> > b/tools/xenstore/xenstored_core.c
> > index fa733e714e9a..b6279bdfe229 100644
> > --- a/tools/xenstore/xenstored_core.c
> > +++ b/tools/xenstore/xenstored_core.c
> > @@ -2065,7 +2065,11 @@ void corrupt(struct connection *conn, const 
> > char *fmt, ...)
> >       va_end(arglist);
> >       log("corruption detected by connection %i: err %s: %s",
> > -        conn ? (int)conn->id : -1, strerror(saved_errno), str);
> > +        conn ? (int)conn->id : -1, strerror(saved_errno),
> > +        str ? str : "ENOMEM");
> > +
> > +    if (str)
> No need for the "if". talloc_free() handles NULL quite fine.
In my original approach, I wasn't checking "if (str)" when I looked at 
talloc_free(), I noticed it would return -1 (i.e. the memory wasn't 
freed) when str is NULL.
I also couldn't find any example in Xenstored where talloc_free() would 
be called with NULL. So I felt it was not a good idea to add one because 
talloc_free() technically return a "failure".
That said, I would not strongly argue to keep it. So I am happy to drop 
the check if that's what you want.
> > +        talloc_free(str);
> >       check_store();
> >   }
> With above fixed:
>
> Reviewed-by: Juergen Gross <jgross@xxxxxxxx>
Thanks!

Cheers,

--
Julien Grall