[PATCH XENSTORE v1 00/10] Code analysis fixes

[PATCH XENSTORE v1 00/10] Code analysis fixes

[Norbert Manthey]

Dear all,

we have been running some code analysis tools on the xenstore code, and triaged
the results. This series presents the robustness fixes we identified.

Best,
Norbert

Michael Kurth (1):
  xenstore: add missing NULL check

Norbert Manthey (9):
  xenstore: add missing NULL check
  xenstore: fix print format string
  xenstore: check formats of trace
  xenstore_client: handle memory on error
  xenstore: handle daemon creation errors
  xenstored: handle port reads correctly
  xenstore: handle do_mkdir and do_rm failure
  xs: handle daemon socket error
  xs: add error handling

 tools/libs/store/xs.c            | 10 +++++++++-
 tools/xenstore/xenstore_client.c |  3 +++
 tools/xenstore/xenstored_core.c  | 16 ++++++++++++++++
 tools/xenstore/xenstored_core.h  |  2 +-
 tools/xenstore/xenstored_posix.c |  6 +++++-
 tools/xenstore/xs_tdb_dump.c     |  6 +++---
 6 files changed, 37 insertions(+), 6 deletions(-)

-- 
2.17.1




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

[PATCH XENSTORE v1 01/10] xenstore: add missing NULL check

[Norbert Manthey]

In case of allocation error, we should not dereference the obtained
NULL pointer. Hence, fail early.

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

Signed-off-by: Norbert Manthey <nmanthey@amazon.de>
Reviewed-by: Thomas Friebel <friebelt@amazon.de>
Reviewed-by: Julien Grall <jgrall@amazon.co.uk>

---
 tools/xenstore/xenstored_core.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -1818,6 +1818,10 @@ static int check_store_(const char *name, struct hashtable *reachable)
 
 		struct hashtable * children =
 			create_hashtable(16, hash_from_key_fn, keys_equal_fn);
+		if (!children) {
+			log("check_store create table: ENOMEM");
+			return ENOMEM;
+		}
 
 		if (!remember_string(reachable, name)) {
 			hashtable_destroy(children, 0);
-- 
2.17.1




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

Re: [PATCH XENSTORE v1 01/10] xenstore: add missing NULL check

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 493 bytes --]

On 26.02.21 15:41, Norbert Manthey wrote:
> In case of allocation error, we should not dereference the obtained
> NULL pointer. Hence, fail early.
> 
> This bug was discovered and resolved using Coverity Static Analysis
> Security Testing (SAST) by Synopsys, Inc.
> 
> Signed-off-by: Norbert Manthey <nmanthey@amazon.de>
> Reviewed-by: Thomas Friebel <friebelt@amazon.de>
> Reviewed-by: Julien Grall <jgrall@amazon.co.uk>

Reviewed-by: Juergen Gross <jgross@suse.com>


Juergen

[-- Attachment #1.1.2: OpenPGP_0xB0DE9DD628BF132F.asc --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

[PATCH XENSTORE v1 01/10] xenstore: add missing NULL check

[Ian Jackson]

Norbert Manthey writes ("[PATCH XENSTORE v1 01/10] xenstore: add missing NULL check"):
> In case of allocation error, we should not dereference the obtained
> NULL pointer. Hence, fail early.
> 
> This bug was discovered and resolved using Coverity Static Analysis
> Security Testing (SAST) by Synopsys, Inc.
> 
> Signed-off-by: Norbert Manthey <nmanthey@amazon.de>
> Reviewed-by: Thomas Friebel <friebelt@amazon.de>
> Reviewed-by: Julien Grall <jgrall@amazon.co.uk>

Release-Acked-by: Ian Jackson <iwj@xenproject.org>

[PATCH XENSTORE v1 02/10] xenstore: fix print format string

[Norbert Manthey]

Use the correct format specifier for unsigned values. Additionally, a
cast was dropped, as the format specifier did not require it anymore.

This was reported by analysis with cppcheck.

Signed-off-by: Norbert Manthey <nmanthey@amazon.de>
Reviewed-by: Thomas Friebel <friebelt@amazon.de>
Reviewed-by: Julien Grall <jgrall@amazon.co.uk>

---
 tools/xenstore/xs_tdb_dump.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/xenstore/xs_tdb_dump.c b/tools/xenstore/xs_tdb_dump.c
--- a/tools/xenstore/xs_tdb_dump.c
+++ b/tools/xenstore/xs_tdb_dump.c
@@ -59,8 +59,8 @@ int main(int argc, char *argv[])
 			fprintf(stderr, "%.*s: BAD truncated\n",
 				(int)key.dsize, key.dptr);
 		else if (data.dsize != total_size(hdr))
-			fprintf(stderr, "%.*s: BAD length %i for %i/%i/%i (%i)\n",
-				(int)key.dsize, key.dptr, (int)data.dsize,
+			fprintf(stderr, "%.*s: BAD length %zu for %u/%u/%u (%u)\n",
+				(int)key.dsize, key.dptr, data.dsize,
 				hdr->num_perms, hdr->datalen,
 				hdr->childlen, total_size(hdr));
 		else {
@@ -69,7 +69,7 @@ int main(int argc, char *argv[])
 
 			printf("%.*s: ", (int)key.dsize, key.dptr);
 			for (i = 0; i < hdr->num_perms; i++)
-				printf("%s%c%i",
+				printf("%s%c%u",
 				       i == 0 ? "" : ",",
 				       perm_to_char(hdr->perms[i].perms),
 				       hdr->perms[i].id);
-- 
2.17.1




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

Re: [PATCH XENSTORE v1 02/10] xenstore: fix print format string

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 464 bytes --]

On 26.02.21 15:41, Norbert Manthey wrote:
> Use the correct format specifier for unsigned values. Additionally, a
> cast was dropped, as the format specifier did not require it anymore.
> 
> This was reported by analysis with cppcheck.
> 
> Signed-off-by: Norbert Manthey <nmanthey@amazon.de>
> Reviewed-by: Thomas Friebel <friebelt@amazon.de>
> Reviewed-by: Julien Grall <jgrall@amazon.co.uk>

Reviewed-by: Juergen Gross <jgross@suse.com>


Juergen

[-- Attachment #1.1.2: OpenPGP_0xB0DE9DD628BF132F.asc --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

[PATCH XENSTORE v1 02/10] xenstore: fix print format string

[Ian Jackson]

Norbert Manthey writes ("[PATCH XENSTORE v1 02/10] xenstore: fix print format string"):
> Use the correct format specifier for unsigned values. Additionally, a
> cast was dropped, as the format specifier did not require it anymore.
> 
> This was reported by analysis with cppcheck.
> 
> Signed-off-by: Norbert Manthey <nmanthey@amazon.de>
> Reviewed-by: Thomas Friebel <friebelt@amazon.de>
> Reviewed-by: Julien Grall <jgrall@amazon.co.uk>

Release-Acked-by: Ian Jackson <iwj@xenproject.org>

[PATCH XENSTORE v1 03/10] xenstore: check formats of trace

[Norbert Manthey]

When passing format strings to the trace function, allow gcc to analyze
those and warn on issues.

Signed-off-by: Norbert Manthey <nmanthey@amazon.de>
Reviewed-by: Thomas Friebel <friebelt@amazon.de>
Reviewed-by: Julien Grall <jgrall@amazon.co.uk>

---
 tools/xenstore/xenstored_core.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/xenstore/xenstored_core.h b/tools/xenstore/xenstored_core.h
--- a/tools/xenstore/xenstored_core.h
+++ b/tools/xenstore/xenstored_core.h
@@ -217,7 +217,7 @@ int delay_request(struct connection *conn, struct buffered_data *in,
 /* Tracing infrastructure. */
 void trace_create(const void *data, const char *type);
 void trace_destroy(const void *data, const char *type);
-void trace(const char *fmt, ...);
+void trace(const char *fmt, ...) __attribute__ ((format (printf, 1, 2)));
 void dtrace_io(const struct connection *conn, const struct buffered_data *data, int out);
 void reopen_log(void);
 void close_log(void);
-- 
2.17.1




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

Re: [PATCH XENSTORE v1 03/10] xenstore: check formats of trace

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 372 bytes --]

On 26.02.21 15:41, Norbert Manthey wrote:
> When passing format strings to the trace function, allow gcc to analyze
> those and warn on issues.
> 
> Signed-off-by: Norbert Manthey <nmanthey@amazon.de>
> Reviewed-by: Thomas Friebel <friebelt@amazon.de>
> Reviewed-by: Julien Grall <jgrall@amazon.co.uk>

Reviewed-by: Juergen Gross <jgross@suse.com>


Juergen


[-- Attachment #1.1.2: OpenPGP_0xB0DE9DD628BF132F.asc --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

[PATCH XENSTORE v1 03/10] xenstore: check formats of trace

[Ian Jackson]

Norbert Manthey writes ("[PATCH XENSTORE v1 03/10] xenstore: check formats of trace"):
> When passing format strings to the trace function, allow gcc to analyze
> those and warn on issues.
> 
> Signed-off-by: Norbert Manthey <nmanthey@amazon.de>
> Reviewed-by: Thomas Friebel <friebelt@amazon.de>
> Reviewed-by: Julien Grall <jgrall@amazon.co.uk>

Release-Acked-by: Ian Jackson <iwj@xenproject.org>

[PATCH XENSTORE v1 04/10] xenstore_client: handle memory on error

[Norbert Manthey]

In case a command fails, also free the memory. As this is for the CLI
client, currently the leaked memory is freed right after receiving the
error, as the application terminates next.

Similarly, if the allocation fails, do not use the NULL pointer
afterwards, but instead error out.

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

Signed-off-by: Norbert Manthey <nmanthey@amazon.de>
Reviewed-by: Thomas Friebel <friebelt@amazon.de>
Reviewed-by: Julien Grall <jgrall@amazon.co.uk>

---
 tools/xenstore/xenstore_client.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tools/xenstore/xenstore_client.c b/tools/xenstore/xenstore_client.c
--- a/tools/xenstore/xenstore_client.c
+++ b/tools/xenstore/xenstore_client.c
@@ -382,11 +382,14 @@ perform(enum mode mode, int optind, int argc, char **argv, struct xs_handle *xsh
                 /* Copy path, because we can't modify argv because we will need it
                    again if xs_transaction_end gives us EAGAIN. */
                 char *p = malloc(strlen(path) + 1);
+                if (!p)
+                    return 1;
                 strcpy(p, path);
                 path = p;
 
             again:
                 if (do_rm(path, xsh, xth)) {
+                    free(path);
                     return 1;
                 }
 
-- 
2.17.1




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

Re: [PATCH XENSTORE v1 04/10] xenstore_client: handle memory on error

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 690 bytes --]

On 26.02.21 15:41, Norbert Manthey wrote:
> In case a command fails, also free the memory. As this is for the CLI
> client, currently the leaked memory is freed right after receiving the
> error, as the application terminates next.
> 
> Similarly, if the allocation fails, do not use the NULL pointer
> afterwards, but instead error out.
> 
> This bug was discovered and resolved using Coverity Static Analysis
> Security Testing (SAST) by Synopsys, Inc.
> 
> Signed-off-by: Norbert Manthey <nmanthey@amazon.de>
> Reviewed-by: Thomas Friebel <friebelt@amazon.de>
> Reviewed-by: Julien Grall <jgrall@amazon.co.uk>

Reviewed-by: Juergen Gross <jgross@suse.com>


Juergen


[-- Attachment #1.1.2: OpenPGP_0xB0DE9DD628BF132F.asc --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH XENSTORE v1 04/10] xenstore_client: handle memory on error

[Ian Jackson]

Norbert Manthey writes ("[PATCH XENSTORE v1 04/10] xenstore_client: handle memory on error"):
> In case a command fails, also free the memory. As this is for the CLI
> client, currently the leaked memory is freed right after receiving the
> error, as the application terminates next.
> 
> Similarly, if the allocation fails, do not use the NULL pointer
> afterwards, but instead error out.

I think this is not for 4.15.

Thanks,
Ian.

[PATCH XENSTORE v1 05/10] xenstore: handle daemon creation errors

[Norbert Manthey]

In rare cases, the path to the daemon socket cannot be created as it is
longer than PATH_MAX. Instead of failing with a NULL pointer dereference,
terminate the application with an error message.

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

Signed-off-by: Norbert Manthey <nmanthey@amazon.de>
Reviewed-by: Thomas Friebel <friebelt@amazon.de>
Reviewed-by: Julien Grall <jgrall@amazon.co.uk>

---
 tools/xenstore/xenstored_core.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -1996,6 +1996,9 @@ static void init_sockets(void)
 	struct sockaddr_un addr;
 	const char *soc_str = xs_daemon_socket();
 
+	if (!soc_str)
+		barf_perror("Failed to obtain xs domain socket");
+
 	/* Create sockets for them to listen to. */
 	atexit(destroy_fds);
 	sock = socket(PF_UNIX, SOCK_STREAM, 0);
-- 
2.17.1




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

Re: [PATCH XENSTORE v1 05/10] xenstore: handle daemon creation errors

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 590 bytes --]

On 26.02.21 15:41, Norbert Manthey wrote:
> In rare cases, the path to the daemon socket cannot be created as it is
> longer than PATH_MAX. Instead of failing with a NULL pointer dereference,
> terminate the application with an error message.
> 
> This bug was discovered and resolved using Coverity Static Analysis
> Security Testing (SAST) by Synopsys, Inc.
> 
> Signed-off-by: Norbert Manthey <nmanthey@amazon.de>
> Reviewed-by: Thomas Friebel <friebelt@amazon.de>
> Reviewed-by: Julien Grall <jgrall@amazon.co.uk>

Reviewed-by: Juergen Gross <jgross@suse.com>


Juergen

[-- Attachment #1.1.2: OpenPGP_0xB0DE9DD628BF132F.asc --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

[PATCH XENSTORE v1 05/10] xenstore: handle daemon creation errors

[Ian Jackson]

Norbert Manthey writes ("[PATCH XENSTORE v1 05/10] xenstore: handle daemon creation errors"):
> In rare cases, the path to the daemon socket cannot be created as it is
> longer than PATH_MAX. Instead of failing with a NULL pointer dereference,
> terminate the application with an error message.
> 
> This bug was discovered and resolved using Coverity Static Analysis
> Security Testing (SAST) by Synopsys, Inc.

Again, not for 4.15 I think.

Ian.

[PATCH XENSTORE v1 06/10] xenstored: handle port reads correctly

[Norbert Manthey]

The read value could be larger than a signed 32bit integer. As -1 is
used as error value, we should not rely on using the full 32 bits.
Hence, when reading the port number, we should make sure we only return
valid values.

This change sanity checks the input.
The issue is that the value for the port is
 1. transmitted as a string, with a fixed amount of digits.
 2. Next, this string is parsed by a function that can deal with strings
    representing 64bit integers
 3. A 64bit integer is returned, and will be truncated to it's lower
    32bits, resulting in a wrong port number (in case the sender of the
    string decides to craft a suitable 64bit value).

The value is typically provided by the kernel, which has this value hard
coded in the proper range. As we use the function strtoul, non-digit
character are considered as end of the input, and hence do not require
checking. Therefore, this change only covers the corner case to make
sure we stay in the 32 bit range.

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

Signed-off-by: Norbert Manthey <nmanthey@amazon.de>
Reviewed-by: Thomas Friebel <friebelt@amazon.de>
Reviewed-by: Julien Grall <jgrall@amazon.co.uk>

---
 tools/xenstore/xenstored_posix.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/tools/xenstore/xenstored_posix.c b/tools/xenstore/xenstored_posix.c
--- a/tools/xenstore/xenstored_posix.c
+++ b/tools/xenstore/xenstored_posix.c
@@ -116,7 +116,7 @@ evtchn_port_t xenbus_evtchn(void)
 {
 	int fd;
 	int rc;
-	evtchn_port_t port;
+	uint64_t port;
 	char str[20];
 
 	fd = open(XENSTORED_PORT_DEV, O_RDONLY);
@@ -136,6 +136,10 @@ evtchn_port_t xenbus_evtchn(void)
 	port = strtoul(str, NULL, 0);
 
 	close(fd);
+
+	if (port >= UINT32_MAX)
+		return -1;
+
 	return port;
 }
 
-- 
2.17.1




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

Re: [PATCH XENSTORE v1 06/10] xenstored: handle port reads correctly

[Andrew Cooper]

On 26/02/2021 14:41, Norbert Manthey wrote:
> The read value could be larger than a signed 32bit integer. As -1 is
> used as error value, we should not rely on using the full 32 bits.
> Hence, when reading the port number, we should make sure we only return
> valid values.
>
> This change sanity checks the input.
> The issue is that the value for the port is
>  1. transmitted as a string, with a fixed amount of digits.
>  2. Next, this string is parsed by a function that can deal with strings
>     representing 64bit integers
>  3. A 64bit integer is returned, and will be truncated to it's lower
>     32bits, resulting in a wrong port number (in case the sender of the
>     string decides to craft a suitable 64bit value).
>
> The value is typically provided by the kernel, which has this value hard
> coded in the proper range. As we use the function strtoul, non-digit
> character are considered as end of the input, and hence do not require
> checking. Therefore, this change only covers the corner case to make
> sure we stay in the 32 bit range.
>
> This bug was discovered and resolved using Coverity Static Analysis
> Security Testing (SAST) by Synopsys, Inc.
>
> Signed-off-by: Norbert Manthey <nmanthey@amazon.de>
> Reviewed-by: Thomas Friebel <friebelt@amazon.de>
> Reviewed-by: Julien Grall <jgrall@amazon.co.uk>

Port numbers are currently limited at 2^17, with easy extension to 2^29
(iirc), but the entire event channel infrastructure would have to
undergo another redesign to get beyond that.

I think we can reasonably make an ABI statement saying that a port
number will never exceed 2^31.  This is already pseudo-encoded in the
evtchn_port_or_error_t mouthful.

~Andrew

Re: [PATCH XENSTORE v1 06/10] xenstored: handle port reads correctly

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 1875 bytes --]

On 26.02.21 16:36, Andrew Cooper wrote:
> On 26/02/2021 14:41, Norbert Manthey wrote:
>> The read value could be larger than a signed 32bit integer. As -1 is
>> used as error value, we should not rely on using the full 32 bits.
>> Hence, when reading the port number, we should make sure we only return
>> valid values.
>>
>> This change sanity checks the input.
>> The issue is that the value for the port is
>>   1. transmitted as a string, with a fixed amount of digits.
>>   2. Next, this string is parsed by a function that can deal with strings
>>      representing 64bit integers
>>   3. A 64bit integer is returned, and will be truncated to it's lower
>>      32bits, resulting in a wrong port number (in case the sender of the
>>      string decides to craft a suitable 64bit value).
>>
>> The value is typically provided by the kernel, which has this value hard
>> coded in the proper range. As we use the function strtoul, non-digit
>> character are considered as end of the input, and hence do not require
>> checking. Therefore, this change only covers the corner case to make
>> sure we stay in the 32 bit range.
>>
>> This bug was discovered and resolved using Coverity Static Analysis
>> Security Testing (SAST) by Synopsys, Inc.
>>
>> Signed-off-by: Norbert Manthey <nmanthey@amazon.de>
>> Reviewed-by: Thomas Friebel <friebelt@amazon.de>
>> Reviewed-by: Julien Grall <jgrall@amazon.co.uk>
> 
> Port numbers are currently limited at 2^17, with easy extension to 2^29
> (iirc), but the entire event channel infrastructure would have to
> undergo another redesign to get beyond that.
> 
> I think we can reasonably make an ABI statement saying that a port
> number will never exceed 2^31.  This is already pseudo-encoded in the
> evtchn_port_or_error_t mouthful.

I agree. There is no need for this patch.


Juergen


[-- Attachment #1.1.2: OpenPGP_0xB0DE9DD628BF132F.asc --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH XENSTORE v1 06/10] xenstored: handle port reads correctly

[Norbert Manthey]

On 3/2/21 6:15 AM, Jürgen Groß wrote:
> On 26.02.21 16:36, Andrew Cooper wrote:
>> On 26/02/2021 14:41, Norbert Manthey wrote:
>>> The read value could be larger than a signed 32bit integer. As -1 is
>>> used as error value, we should not rely on using the full 32 bits.
>>> Hence, when reading the port number, we should make sure we only return
>>> valid values.
>>>
>>> This change sanity checks the input.
>>> The issue is that the value for the port is
>>>   1. transmitted as a string, with a fixed amount of digits.
>>>   2. Next, this string is parsed by a function that can deal with
>>> strings
>>>      representing 64bit integers
>>>   3. A 64bit integer is returned, and will be truncated to it's lower
>>>      32bits, resulting in a wrong port number (in case the sender of
>>> the
>>>      string decides to craft a suitable 64bit value).
>>>
>>> The value is typically provided by the kernel, which has this value
>>> hard
>>> coded in the proper range. As we use the function strtoul, non-digit
>>> character are considered as end of the input, and hence do not require
>>> checking. Therefore, this change only covers the corner case to make
>>> sure we stay in the 32 bit range.
>>>
>>> This bug was discovered and resolved using Coverity Static Analysis
>>> Security Testing (SAST) by Synopsys, Inc.
>>>
>>> Signed-off-by: Norbert Manthey <nmanthey@amazon.de>
>>> Reviewed-by: Thomas Friebel <friebelt@amazon.de>
>>> Reviewed-by: Julien Grall <jgrall@amazon.co.uk>
>>
>> Port numbers are currently limited at 2^17, with easy extension to 2^29
>> (iirc), but the entire event channel infrastructure would have to
>> undergo another redesign to get beyond that.
>>
>> I think we can reasonably make an ABI statement saying that a port
>> number will never exceed 2^31.  This is already pseudo-encoded in the
>> evtchn_port_or_error_t mouthful.
>
> I agree. There is no need for this patch.

I understand, and am fine with dropping this patch.

Out of curiosity, if the actual limit is lower than what the patch
currently enforces, would it make sense to adapt the bound check to that
number?

Best,
Norbert

>
>
> Juergen
>




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

Re: [PATCH XENSTORE v1 06/10] xenstored: handle port reads correctly

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 2770 bytes --]

On 02.03.21 08:48, Norbert Manthey wrote:
> On 3/2/21 6:15 AM, Jürgen Groß wrote:
>> On 26.02.21 16:36, Andrew Cooper wrote:
>>> On 26/02/2021 14:41, Norbert Manthey wrote:
>>>> The read value could be larger than a signed 32bit integer. As -1 is
>>>> used as error value, we should not rely on using the full 32 bits.
>>>> Hence, when reading the port number, we should make sure we only return
>>>> valid values.
>>>>
>>>> This change sanity checks the input.
>>>> The issue is that the value for the port is
>>>>    1. transmitted as a string, with a fixed amount of digits.
>>>>    2. Next, this string is parsed by a function that can deal with
>>>> strings
>>>>       representing 64bit integers
>>>>    3. A 64bit integer is returned, and will be truncated to it's lower
>>>>       32bits, resulting in a wrong port number (in case the sender of
>>>> the
>>>>       string decides to craft a suitable 64bit value).
>>>>
>>>> The value is typically provided by the kernel, which has this value
>>>> hard
>>>> coded in the proper range. As we use the function strtoul, non-digit
>>>> character are considered as end of the input, and hence do not require
>>>> checking. Therefore, this change only covers the corner case to make
>>>> sure we stay in the 32 bit range.
>>>>
>>>> This bug was discovered and resolved using Coverity Static Analysis
>>>> Security Testing (SAST) by Synopsys, Inc.
>>>>
>>>> Signed-off-by: Norbert Manthey <nmanthey@amazon.de>
>>>> Reviewed-by: Thomas Friebel <friebelt@amazon.de>
>>>> Reviewed-by: Julien Grall <jgrall@amazon.co.uk>
>>>
>>> Port numbers are currently limited at 2^17, with easy extension to 2^29
>>> (iirc), but the entire event channel infrastructure would have to
>>> undergo another redesign to get beyond that.
>>>
>>> I think we can reasonably make an ABI statement saying that a port
>>> number will never exceed 2^31.  This is already pseudo-encoded in the
>>> evtchn_port_or_error_t mouthful.
>>
>> I agree. There is no need for this patch.
> 
> I understand, and am fine with dropping this patch.
> 
> Out of curiosity, if the actual limit is lower than what the patch
> currently enforces, would it make sense to adapt the bound check to that
> number?

No, I don't think so. Especially as the boundary to check against isn't
known by Xenstore (the boundary value depends on 2-level or fifo events
being used, and this information is not exported to user land).

The value is coming from the kernel, and it is used with another kernel
interface. So if the kernel wants to play dirty tricks with Xenstore, it
doesn't need to deliver a wrong event channel number, it can just play
those games in the event channel driver.


Juergen

[-- Attachment #1.1.2: OpenPGP_0xB0DE9DD628BF132F.asc --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

[PATCH XENSTORE v1 07/10] xenstore: handle do_mkdir and do_rm failure

[Norbert Manthey]

In the out of memory case, we might return a NULL pointer when
canonicalizing node names. This NULL pointer is not checked when
creating a directory, or when removing a node. This change handles
the NULL pointer for these two cases.

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

Signed-off-by: Norbert Manthey <nmanthey@amazon.de>
Reviewed-by: Thomas Friebel <friebelt@amazon.de>
Reviewed-by: Julien Grall <jgrall@amazon.co.uk>

---
 tools/xenstore/xenstored_core.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -1160,6 +1160,8 @@ static int do_mkdir(struct connection *conn, struct buffered_data *in)
 		/* No permissions? */
 		if (errno != ENOENT)
 			return errno;
+		if (!name)
+			return ENOMEM;
 		node = create_node(conn, in, name, NULL, 0);
 		if (!node)
 			return errno;
@@ -1274,6 +1276,8 @@ static int do_rm(struct connection *conn, struct buffered_data *in)
 	if (!node) {
 		/* Didn't exist already?  Fine, if parent exists. */
 		if (errno == ENOENT) {
+			if (!name)
+				return ENOMEM;
 			parentname = get_parent(in, name);
 			if (!parentname)
 				return errno;
-- 
2.17.1




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

Re: [PATCH XENSTORE v1 07/10] xenstore: handle do_mkdir and do_rm failure

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 631 bytes --]

On 26.02.21 15:41, Norbert Manthey wrote:
> In the out of memory case, we might return a NULL pointer when
> canonicalizing node names. This NULL pointer is not checked when
> creating a directory, or when removing a node. This change handles
> the NULL pointer for these two cases.
> 
> This bug was discovered and resolved using Coverity Static Analysis
> Security Testing (SAST) by Synopsys, Inc.
> 
> Signed-off-by: Norbert Manthey <nmanthey@amazon.de>
> Reviewed-by: Thomas Friebel <friebelt@amazon.de>
> Reviewed-by: Julien Grall <jgrall@amazon.co.uk>

Reviewed-by: Juergen Gross <jgross@suse.com>


Juergen

[-- Attachment #1.1.2: OpenPGP_0xB0DE9DD628BF132F.asc --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

[PATCH XENSTORE v1 07/10] xenstore: handle do_mkdir and do_rm failure

[Ian Jackson]

Norbert Manthey writes ("[PATCH XENSTORE v1 07/10] xenstore: handle do_mkdir and do_rm failure"):
> In the out of memory case, we might return a NULL pointer when
> canonicalizing node names. This NULL pointer is not checked when
> creating a directory, or when removing a node. This change handles
> the NULL pointer for these two cases.
> 
> This bug was discovered and resolved using Coverity Static Analysis
> Security Testing (SAST) by Synopsys, Inc.
> 
> Signed-off-by: Norbert Manthey <nmanthey@amazon.de>
> Reviewed-by: Thomas Friebel <friebelt@amazon.de>
> Reviewed-by: Julien Grall <jgrall@amazon.co.uk>

Release-Acked-by: Ian Jackson <iwj@xenproject.org>

[PATCH XENSTORE v1 08/10] xenstore: add missing NULL check

[Norbert Manthey]

From: Michael Kurth <mku@amazon.com>

In case of allocation error, we should not dereference the obtained
NULL pointer.

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

Signed-off-by: Michael Kurth <mku@amazon.com>
Signed-off-by: Norbert Manthey <nmanthey@amazon.de>
Reviewed-by: Thomas Friebel <friebelt@amazon.de>
Reviewed-by: Julien Grall <jgrall@amazon.co.uk>

---
 tools/xenstore/xenstored_core.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -504,6 +504,11 @@ int write_node_raw(struct connection *conn, TDB_DATA *key, struct node *node,
 	}
 
 	data.dptr = talloc_size(node, data.dsize);
+	if (!data.dptr) {
+		errno = ENOMEM;
+		return errno;
+	}
+
 	hdr = (void *)data.dptr;
 	hdr->generation = node->generation;
 	hdr->num_perms = node->perms.num;
-- 
2.17.1




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

Re: [PATCH XENSTORE v1 08/10] xenstore: add missing NULL check

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 567 bytes --]

On 26.02.21 15:41, Norbert Manthey wrote:
> From: Michael Kurth <mku@amazon.com>
> 
> In case of allocation error, we should not dereference the obtained
> NULL pointer.
> 
> This bug was discovered and resolved using Coverity Static Analysis
> Security Testing (SAST) by Synopsys, Inc.
> 
> Signed-off-by: Michael Kurth <mku@amazon.com>
> Signed-off-by: Norbert Manthey <nmanthey@amazon.de>
> Reviewed-by: Thomas Friebel <friebelt@amazon.de>
> Reviewed-by: Julien Grall <jgrall@amazon.co.uk>

Reviewed-by: Juergen Gross <jgross@suse.com>


Juergen

[-- Attachment #1.1.2: OpenPGP_0xB0DE9DD628BF132F.asc --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

[PATCH XENSTORE v1 08/10] xenstore: add missing NULL check

[Ian Jackson]

Norbert Manthey writes ("[PATCH XENSTORE v1 08/10] xenstore: add missing NULL check"):
> From: Michael Kurth <mku@amazon.com>
> 
> In case of allocation error, we should not dereference the obtained
> NULL pointer.
> 
> This bug was discovered and resolved using Coverity Static Analysis
> Security Testing (SAST) by Synopsys, Inc.
> 
> Signed-off-by: Michael Kurth <mku@amazon.com>
> Signed-off-by: Norbert Manthey <nmanthey@amazon.de>
> Reviewed-by: Thomas Friebel <friebelt@amazon.de>
> Reviewed-by: Julien Grall <jgrall@amazon.co.uk>

Release-Acked-by: Ian Jackson <iwj@xenproject.org>

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

[Norbert Manthey]

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 <nmanthey@amazon.de>
Reviewed-by: Thomas Friebel <friebelt@amazon.de>
Reviewed-by: Julien Grall <jgrall@amazon.co.uk>

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

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;
-- 
2.17.1




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

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

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 758 bytes --]

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 <nmanthey@amazon.de>
> Reviewed-by: Thomas Friebel <friebelt@amazon.de>
> Reviewed-by: Julien Grall <jgrall@amazon.co.uk>

Reviewed-by: Juergen Gross <jgross@suse.com>


Juergen


[-- Attachment #1.1.2: OpenPGP_0xB0DE9DD628BF132F.asc --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

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

[Ian Jackson]

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.

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

[Norbert Manthey]

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

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

[Ian Jackson]

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 XENSTORE v1 10/10] xs: add error handling

[Norbert Manthey]

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 <nmanthey@amazon.de>
Reviewed-by: Julien Grall <jgrall@amazon.co.uk>
Reviewed-by: Raphael Ning <raphning@amazon.co.uk>

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

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;
-- 
2.17.1




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

Re: [PATCH XENSTORE v1 10/10] xs: add error handling

[Julien Grall]

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 <nmanthey@amazon.de>
> Reviewed-by: Julien Grall <jgrall@amazon.co.uk
... you carried over my reviewed-by tag.


Cheers,

-- 
Julien Grall

Re: [PATCH XENSTORE v1 10/10] xs: add error handling

[Norbert Manthey]

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 <nmanthey@amazon.de>
>> Reviewed-by: Julien Grall <jgrall@amazon.co.uk
> ... 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

Re: [PATCH XENSTORE v1 10/10] xs: add error handling

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 556 bytes --]

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 <nmanthey@amazon.de>
> Reviewed-by: Julien Grall <jgrall@amazon.co.uk>
> Reviewed-by: Raphael Ning <raphning@amazon.co.uk>

Reviewed-by: Juergen Gross <jgross@suse.com>


Juergen

[-- Attachment #1.1.2: OpenPGP_0xB0DE9DD628BF132F.asc --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH XENSTORE v1 10/10] xs: add error handling

[Ian Jackson]

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 <nmanthey@amazon.de>
> Reviewed-by: Julien Grall <jgrall@amazon.co.uk>
> Reviewed-by: Raphael Ning <raphning@amazon.co.uk>

Release-Acked-by: Ian Jackson <iwj@xenproject.org>

[for-4.15] Re: [PATCH XENSTORE v1 00/10] Code analysis fixes

[Julien Grall]

Hi,

I have tagged the e-mail with 4.15 as I think we likely want some of the 
patches to be in the next release.

As a minimum, we get the following:
   - patch #7: xenstore: handle do_mkdir and do_rm failure
   - patch #8: xenstore: add missing NULL check
   - patch #10: xs: add error handling

The first two add missing NULL check in runtime code in XenStored. The 
3rd one adds a missing NULL check in xs_is_domain_introduced() in 
libxenstore (can be used at runtime by xenpaging at least).

In addition to that, I would like to consider patch #3: xenstore: check 
formats of trace. It is allowing the compiler to check the format printf 
for trace(). This should be low-risk.

For the rest is a mix of silencing coverity and potential errors either 
at init or in a standalone binaries.

The init ones would be useful (patch #1, #5, #9) for Xenstored 
LiveUpdate as they would be potential triggered when upgrading the 
binary. But I am not sure whether we consider LiveUpdate supported.

Any thoughts?

Cheers,

On 26/02/2021 14:41, Norbert Manthey wrote:
> Dear all,
> 
> we have been running some code analysis tools on the xenstore code, and triaged
> the results. This series presents the robustness fixes we identified.
> 
> Best,
> Norbert
> 
> Michael Kurth (1):
>    xenstore: add missing NULL check
> 
> Norbert Manthey (9):
>    xenstore: add missing NULL check
>    xenstore: fix print format string
>    xenstore: check formats of trace
>    xenstore_client: handle memory on error
>    xenstore: handle daemon creation errors
>    xenstored: handle port reads correctly
>    xenstore: handle do_mkdir and do_rm failure
>    xs: handle daemon socket error
>    xs: add error handling
> 
>   tools/libs/store/xs.c            | 10 +++++++++-
>   tools/xenstore/xenstore_client.c |  3 +++
>   tools/xenstore/xenstored_core.c  | 16 ++++++++++++++++
>   tools/xenstore/xenstored_core.h  |  2 +-
>   tools/xenstore/xenstored_posix.c |  6 +++++-
>   tools/xenstore/xs_tdb_dump.c     |  6 +++---
>   6 files changed, 37 insertions(+), 6 deletions(-)
> 

-- 
Julien Grall

Re: [for-4.15] Re: [PATCH XENSTORE v1 00/10] Code analysis fixes

[Andrew Cooper]

On 01/03/2021 18:01, Julien Grall wrote:
> Hi,
>
> I have tagged the e-mail with 4.15 as I think we likely want some of
> the patches to be in the next release.
>
> As a minimum, we get the following:
>   - patch #7: xenstore: handle do_mkdir and do_rm failure
>   - patch #8: xenstore: add missing NULL check
>   - patch #10: xs: add error handling
>
> The first two add missing NULL check in runtime code in XenStored. The
> 3rd one adds a missing NULL check in xs_is_domain_introduced() in
> libxenstore (can be used at runtime by xenpaging at least).
>
> In addition to that, I would like to consider patch #3: xenstore:
> check formats of trace. It is allowing the compiler to check the
> format printf for trace(). This should be low-risk.
>
> For the rest is a mix of silencing coverity and potential errors
> either at init or in a standalone binaries.
>
> The init ones would be useful (patch #1, #5, #9) for Xenstored
> LiveUpdate as they would be potential triggered when upgrading the
> binary. But I am not sure whether we consider LiveUpdate supported.
>
> Any thoughts?

Live Update is a headline feature, even if only tech preview.

I'd say that all bugfixes are fair game, and low risk.  All these fixes
(other than the evtchn one which has an outstanding question) look to be
reasonable to take.  They're all simple and obvious fixes pointed out by
static analysis.

~Andrew

Re: [for-4.15] Re: [PATCH XENSTORE v1 00/10] Code analysis fixes

[Julien Grall]

Hi Andrew,

On 01/03/2021 19:39, Andrew Cooper wrote:
> On 01/03/2021 18:01, Julien Grall wrote:
>> Hi,
>>
>> I have tagged the e-mail with 4.15 as I think we likely want some of
>> the patches to be in the next release.
>>
>> As a minimum, we get the following:
>>    - patch #7: xenstore: handle do_mkdir and do_rm failure
>>    - patch #8: xenstore: add missing NULL check
>>    - patch #10: xs: add error handling
>>
>> The first two add missing NULL check in runtime code in XenStored. The
>> 3rd one adds a missing NULL check in xs_is_domain_introduced() in
>> libxenstore (can be used at runtime by xenpaging at least).
>>
>> In addition to that, I would like to consider patch #3: xenstore:
>> check formats of trace. It is allowing the compiler to check the
>> format printf for trace(). This should be low-risk.
>>
>> For the rest is a mix of silencing coverity and potential errors
>> either at init or in a standalone binaries.
>>
>> The init ones would be useful (patch #1, #5, #9) for Xenstored
>> LiveUpdate as they would be potential triggered when upgrading the
>> binary. But I am not sure whether we consider LiveUpdate supported.
>>
>> Any thoughts?
> 
> Live Update is a headline feature, even if only tech preview.

I thought it was a tech preview but I couldn't find the statement in 
SUPPORT.MD. I guess we should update it before 4.15 is released.

> 
> I'd say that all bugfixes are fair game, and low risk.  All these fixes
> (other than the evtchn one which has an outstanding question) look to be
> reasonable to take.  They're all simple and obvious fixes pointed out by
> static analysis.

That's a fair point. I wanted to set an order as I know the rules are 
getting tighter. So this gives an opportunity to Ian to have enough data 
to decide what's the best.

Cheers,

-- 
Julien Grall

Re: [PATCH XENSTORE v1 00/10] Code analysis fixes

[Julien Grall]

Hi Norbert,

Thank you for the patches. Please find below a state for each patches.

On 26/02/2021 14:41, Norbert Manthey wrote:

For the following patches:

>    xenstore: add missing NULL check
>    xenstore: fix print format string
 >    xenstore: check formats of trace
 >    xenstore: handle do_mkdir and do_rm failure
 >    xenstore: add missing NULL check
 >    xs: add error handling

They are fully reviewed and Ian provided a release-acked-by. So I have 
merged them to staging.

Note that last one was merged with the commit message/title tweaked.

For the following patches:

>    xenstore_client: handle memory on error
>    xenstore: handle daemon creation errors

They are fully reviewed but so far Ian didn't provided a 
release-acked-by. If you (or someone else) think they should be merged, 
then please reply on each patch.

For now, I have merged them to my for-next/4.16 branch [1]. The patches 
will be folded in staging when the tree is re-opened.

For the following patch:

>    xenstored: handle port reads correctly

IIUC, this was dropped.

For the following patch:

>    xs: handle daemon socket error

Ian had one question about it. I haven't committed it in any branch for now.

Cheers,

[1] 
https://xenbits.xen.org/gitweb/?p=people/julieng/xen-unstable.git;a=shortlog;h=refs/heads/for-next/4.16

-- 
Julien Grall