| Message ID | 20220623112407.13604-1-julien@xen.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | tools/xenstored: Harden corrupt() | expand |
On 23.06.22 13:24, Julien Grall wrote: > From: Julien Grall <jgrall@amazon.com> > > 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@amazon.com> > --- > 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. > + talloc_free(str); > > check_store(); > } With above fixed: Reviewed-by: Juergen Gross <jgross@suse.com> Juergen
Hi Juergen, On 23/06/2022 12:28, Juergen Gross wrote: > On 23.06.22 13:24, Julien Grall wrote: >> From: Julien Grall <jgrall@amazon.com> >> >> 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@amazon.com> >> --- >> 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@suse.com> Thanks! Cheers,
On 23/06/2022 12:28, Juergen Gross wrote: > On 23.06.22 13:24, Julien Grall wrote: >> From: Julien Grall <jgrall@amazon.com> >> >> 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@amazon.com> >> --- >> 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" seeing as we use this idiom in a lot of places. ~Andrew
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@amazon.com> >>> >>> 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@amazon.com> >>> --- >>> 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,
On 23.06.2022 13:24, Julien Grall wrote: > From: Julien Grall <jgrall@amazon.com> > > 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@amazon.com> Is this something which would want queuing for backport? Jan
On 23/06/2022 13:59, Jan Beulich wrote: > On 23.06.2022 13:24, Julien Grall wrote: >> From: Julien Grall <jgrall@amazon.com> >> >> 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@amazon.com> > > Is this something which would want queuing for backport? I would say yes. There are a couple of more Xenstored patches I would consider for backporting: fe9be76d880b tools/xenstore: fix error handling of check_store() b977929d3646 tools/xenstore: fix hashtable_expand() zeroing new area Who is taking care of tools backport nowadays? Cheers,
On 23.06.2022 15:03, Julien Grall wrote: > > > On 23/06/2022 13:59, Jan Beulich wrote: >> On 23.06.2022 13:24, Julien Grall wrote: >>> From: Julien Grall <jgrall@amazon.com> >>> >>> 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@amazon.com> >> >> Is this something which would want queuing for backport? > > I would say yes. There are a couple of more Xenstored patches I would > consider for backporting: > > fe9be76d880b tools/xenstore: fix error handling of check_store() > b977929d3646 tools/xenstore: fix hashtable_expand() zeroing new area > > Who is taking care of tools backport nowadays? I'm trying to, as long as they apply cleanly enough. But I'd prefer if rather sooner then later I could offload this again. And I'm not actively looking to spot backporting candidates there (unlike for the hypervisor, excluding Arm). Jan
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) + talloc_free(str); check_store(); }