[XENSTORE,v1,09/10] xs: handle daemon socket error

Commit Message

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

When starting the daemon, we might see a NULL pointer instead of the
path to the socket.

Only relevant in case we start the process in a very deep directory
path, with a length close to 4096 so that appending "/socket" would
exceed the limit. Hence, such an error is unlikely, but should still be
fixed to not result in a NULL pointer dereference.

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: Thomas Friebel <[e-mail]>
Reviewed-by: Julien Grall <[e-mail]>

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

Comments

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

On 26.02.21 15:41, Norbert Manthey wrote:
> When starting the daemon, we might see a NULL pointer instead of the
> path to the socket.
> 
> Only relevant in case we start the process in a very deep directory
> path, with a length close to 4096 so that appending "/socket" would
> exceed the limit. Hence, such an error is unlikely, but should still be
> fixed to not result in a NULL pointer dereference.
> 
> 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: Thomas Friebel <[e-mail]>
> Reviewed-by: Julien Grall <[e-mail]>

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


Juergen

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

Norbert Manthey writes ("[PATCH XENSTORE v1 09/10] xs: handle daemon socket error"):
> When starting the daemon, we might see a NULL pointer instead of the
> path to the socket.
> 
> Only relevant in case we start the process in a very deep directory
> path, with a length close to 4096 so that appending "/socket" would
> exceed the limit. Hence, such an error is unlikely, but should still be
> fixed to not result in a NULL pointer dereference.

This description talks about starting the daemon ...

> ---
>  tools/libs/store/xs.c | 3 +++
>  1 file changed, 3 insertions(+)

But I think ...

> +	if (!connect_to)
> +		return NULL;
> +

... this is client code ?

Apologies if I am confused.

Ian.

Norbert Manthey March 4, 2021, 2:58 p.m. UTC | #3

On 3/3/21 5:13 PM, Ian Jackson wrote:
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
>
>
>
> Norbert Manthey writes ("[PATCH XENSTORE v1 09/10] xs: handle daemon socket error"):
>> When starting the daemon, we might see a NULL pointer instead of the
>> path to the socket.
This first sentence could be more specific, i.e.:

When connecting to the deamon in xs_open, the functions that return the
socket or device location might return NULL in corner cases.
>>
>> Only relevant in case we start the process in a very deep directory
>> path, with a length close to 4096 so that appending "/socket" would
>> exceed the limit. Hence, such an error is unlikely, but should still be
>> fixed to not result in a NULL pointer dereference.
> This description talks about starting the daemon ...
>
>> ---
>>  tools/libs/store/xs.c | 3 +++
>>  1 file changed, 3 insertions(+)
> But I think ...
>
>> +     if (!connect_to)
>> +             return NULL;
>> +
> ... this is client code ?

This is client code, yes. The patched 'get_handle' function receives the
parameter 'connect_to' in the function xs_open. There, the value of the
functions 'xs_deamon_socket_ro', 'xs_deamon_socket' and 'xs_domain_dev'
are passed to this function, without checking for the value NULL.

I agree that the description might be confusing, as the fix is applied
to a function that does not cause the actual problem. How about
rephrasing the first part of the commit message to the above proposal?

Best,
Norbert

>
> Apologies if I am confused.
>
> Ian.




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

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

Norbert Manthey writes ("Re: [PATCH XENSTORE v1 09/10] xs: handle daemon socket error"):
> On 3/3/21 5:13 PM, Ian Jackson wrote:
> > Norbert Manthey writes ("[PATCH XENSTORE v1 09/10] xs: handle daemon socket error"):
> >> When starting the daemon, we might see a NULL pointer instead of the
> >> path to the socket.
..
> > ... this is client code ?
> 
> This is client code, yes. The patched 'get_handle' function receives the
> parameter 'connect_to' in the function xs_open. There, the value of the
> functions 'xs_deamon_socket_ro', 'xs_deamon_socket' and 'xs_domain_dev'
> are passed to this function, without checking for the value NULL.
> 
> I agree that the description might be confusing, as the fix is applied
> to a function that does not cause the actual problem. How about
> rephrasing the first part of the commit message to the above proposal?

Improving the commit message seems to me to be needed in any case
since as far as I can tell from what I read here, the existing one is
actualy wrong :-).

With my maintainer/reviewer hat on, I think this new exit path
involves returning an error from this function without setting errno,
so it's wrong ?

As for the release, I think this is a very low-impact bug and now it
seems not 100% obvious, so unless someone would like to explain why it
should go into 4.15 I'd like to see it in -next.

Thanks,
Ian.

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
@@ -240,6 +240,9 @@  static struct xs_handle *get_handle(const char *connect_to)
 	struct xs_handle *h = NULL;
 	int saved_errno;
 
+	if (!connect_to)
+		return NULL;
+
 	h = malloc(sizeof(*h));
 	if (h == NULL)
 		goto err;