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

[Juergen Gross <jgross@xxxxxxxx>]

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.

> +               talloc_free(str);
>   
>   	check_store();
>   }
With above fixed:

Reviewed-by: Juergen Gross <jgross@xxxxxxxx>


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature