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

[Julien Grall <julien@xxxxxxx>]

Hi Andrew,

On 23/06/2022 13:41, Andrew Cooper wrote:
> 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");
> str ?: "ENOMEM"
Sure. I have updated the patch and committed it.

Cheers,

--
Julien Grall