[XENSTORE,v1,10/10] xs: add error handling

Commit Message

Norbert Manthey Feb. 26, 2021, 2:41 p.m. UTC

In case of a failure deep in the call tree, we might return NULL as the
value of the domain. In that case, error out instead of dereferencing
the NULL pointer.

This bug was discovered and resolved using Coverity Static Analysis
Security Testing (SAST) by Synopsys, Inc.

Signed-off-by: Norbert Manthey <[e-mail]>
Reviewed-by: Julien Grall <[e-mail]>
Reviewed-by: Raphael Ning <[e-mail]>

---
 tools/libs/store/xs.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Comments

Julien Grall Feb. 26, 2021, 2:53 p.m. UTC | #1

Hi Norbert,

On 26/02/2021 14:41, Norbert Manthey wrote:
> In case of a failure deep in the call tree, we might return NULL as the
> value of the domain. In that case, error out instead of dereferencing
> the NULL pointer.
> 
> This bug was discovered and resolved using Coverity Static Analysis
> Security Testing (SAST) by Synopsys, Inc.

This commit message is not very descriptive. Internally, I suggested:

"
tools/xenstore: Harden xs_domain_is_introduced()

The function single_with_domid() may return NULL if something
went wrong (e.g. XenStored returns an error or the connection is
in bad state).

They are unlikely but not impossible, so it would be better to
return an error and allow the caller to handle it gracefully rather
than crashing.

In this case we should treat it as the domain has disappeared (i.e.
return false) as the caller will not likely going to be able to
communicate with XenStored again.

This bug was discovered and resolved using Coverity Static Analysis
Security Testing (SAST) by Synopsys, Inc.
"

I would have expected this to be addressed given that...

> 
> Signed-off-by: Norbert Manthey <[e-mail]>
> Reviewed-by: Julien Grall <[e-mail]
... you carried over my reviewed-by tag.


Cheers,

Norbert Manthey Feb. 26, 2021, 3:26 p.m. UTC | #2

On 2/26/21 3:53 PM, Julien Grall wrote:
> Hi Norbert,
>
> On 26/02/2021 14:41, Norbert Manthey wrote:
>> In case of a failure deep in the call tree, we might return NULL as the
>> value of the domain. In that case, error out instead of dereferencing
>> the NULL pointer.
>>
>> This bug was discovered and resolved using Coverity Static Analysis
>> Security Testing (SAST) by Synopsys, Inc.
>
> This commit message is not very descriptive. Internally, I suggested:
>
> "
> tools/xenstore: Harden xs_domain_is_introduced()
>
> The function single_with_domid() may return NULL if something
> went wrong (e.g. XenStored returns an error or the connection is
> in bad state).
>
> They are unlikely but not impossible, so it would be better to
> return an error and allow the caller to handle it gracefully rather
> than crashing.
>
> In this case we should treat it as the domain has disappeared (i.e.
> return false) as the caller will not likely going to be able to
> communicate with XenStored again.
>
> This bug was discovered and resolved using Coverity Static Analysis
> Security Testing (SAST) by Synopsys, Inc.
> "
>
> I would have expected this to be addressed given that...

Understood. I will iterate.

Norbert

>
>>
>> Signed-off-by: Norbert Manthey <[e-mail]>
>> Reviewed-by: Julien Grall <[e-mail]
> ... you carried over my reviewed-by tag.
>
>
> Cheers,
>
> -- 
> Julien Grall



Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879

Jürgen Groß March 2, 2021, 5:11 a.m. UTC | #3

On 26.02.21 15:41, Norbert Manthey wrote:
> In case of a failure deep in the call tree, we might return NULL as the
> value of the domain. In that case, error out instead of dereferencing
> the NULL pointer.
> 
> This bug was discovered and resolved using Coverity Static Analysis
> Security Testing (SAST) by Synopsys, Inc.
> 
> Signed-off-by: Norbert Manthey <[e-mail]>
> Reviewed-by: Julien Grall <[e-mail]>
> Reviewed-by: Raphael Ning <[e-mail]>

Reviewed-by: Juergen Gross <[e-mail]>


Juergen

Ian Jackson March 3, 2021, 4:13 p.m. UTC | #4

Norbert Manthey writes ("[PATCH XENSTORE v1 10/10] xs: add error handling"):
> In case of a failure deep in the call tree, we might return NULL as the
> value of the domain. In that case, error out instead of dereferencing
> the NULL pointer.
> 
> This bug was discovered and resolved using Coverity Static Analysis
> Security Testing (SAST) by Synopsys, Inc.
> 
> Signed-off-by: Norbert Manthey <[e-mail]>
> Reviewed-by: Julien Grall <[e-mail]>
> Reviewed-by: Raphael Ning <[e-mail]>

Release-Acked-by: Ian Jackson <[e-mail]>

Patch

diff --git a/tools/libs/store/xs.c b/tools/libs/store/xs.c
--- a/tools/libs/store/xs.c
+++ b/tools/libs/store/xs.c
@@ -1183,7 +1183,12 @@  bool xs_path_is_subpath(const char *parent, const char *child)
 bool xs_is_domain_introduced(struct xs_handle *h, unsigned int domid)
 {
 	char *domain = single_with_domid(h, XS_IS_DOMAIN_INTRODUCED, domid);
-	int rc = strcmp("F", domain);
+	bool rc = false;
+
+	if (!domain)
+		return rc;
+
+	rc = strcmp("F", domain) != 0;
 
 	free(domain);
 	return rc;