* [PATCH XENSTORE v1 00/10] Code analysis fixes
@ 2021-02-26 14:41 Norbert Manthey
2021-02-26 14:41 ` [PATCH XENSTORE v1 01/10] xenstore: add missing NULL check Norbert Manthey
` (11 more replies)
0 siblings, 12 replies; 41+ messages in thread
From: Norbert Manthey @ 2021-02-26 14:41 UTC (permalink / raw)
To: xen-devel
Cc: Ian Jackson, Juergen Gross, Wei Liu, Julien Grall, Michael Kurth,
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
^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH XENSTORE v1 01/10] xenstore: add missing NULL check
2021-02-26 14:41 [PATCH XENSTORE v1 00/10] Code analysis fixes Norbert Manthey
@ 2021-02-26 14:41 ` Norbert Manthey
2021-03-02 5:07 ` Jürgen Groß
2021-03-03 16:07 ` Ian Jackson
2021-02-26 14:41 ` [PATCH XENSTORE v1 02/10] xenstore: fix print format string Norbert Manthey
` (10 subsequent siblings)
11 siblings, 2 replies; 41+ messages in thread
From: Norbert Manthey @ 2021-02-26 14:41 UTC (permalink / raw)
To: xen-devel
Cc: Ian Jackson, Juergen Gross, Wei Liu, Julien Grall, Michael Kurth,
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
^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH XENSTORE v1 02/10] xenstore: fix print format string
2021-02-26 14:41 [PATCH XENSTORE v1 00/10] Code analysis fixes Norbert Manthey
2021-02-26 14:41 ` [PATCH XENSTORE v1 01/10] xenstore: add missing NULL check Norbert Manthey
@ 2021-02-26 14:41 ` Norbert Manthey
2021-03-02 5:07 ` Jürgen Groß
2021-03-03 16:08 ` Ian Jackson
2021-02-26 14:41 ` [PATCH XENSTORE v1 03/10] xenstore: check formats of trace Norbert Manthey
` (9 subsequent siblings)
11 siblings, 2 replies; 41+ messages in thread
From: Norbert Manthey @ 2021-02-26 14:41 UTC (permalink / raw)
To: xen-devel
Cc: Ian Jackson, Juergen Gross, Wei Liu, Julien Grall, Michael Kurth,
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
^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH XENSTORE v1 03/10] xenstore: check formats of trace
2021-02-26 14:41 [PATCH XENSTORE v1 00/10] Code analysis fixes Norbert Manthey
2021-02-26 14:41 ` [PATCH XENSTORE v1 01/10] xenstore: add missing NULL check Norbert Manthey
2021-02-26 14:41 ` [PATCH XENSTORE v1 02/10] xenstore: fix print format string Norbert Manthey
@ 2021-02-26 14:41 ` Norbert Manthey
2021-03-02 5:08 ` Jürgen Groß
2021-03-03 16:08 ` Ian Jackson
2021-02-26 14:41 ` [PATCH XENSTORE v1 04/10] xenstore_client: handle memory on error Norbert Manthey
` (8 subsequent siblings)
11 siblings, 2 replies; 41+ messages in thread
From: Norbert Manthey @ 2021-02-26 14:41 UTC (permalink / raw)
To: xen-devel
Cc: Ian Jackson, Juergen Gross, Wei Liu, Julien Grall, Michael Kurth,
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
^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH XENSTORE v1 04/10] xenstore_client: handle memory on error
2021-02-26 14:41 [PATCH XENSTORE v1 00/10] Code analysis fixes Norbert Manthey
` (2 preceding siblings ...)
2021-02-26 14:41 ` [PATCH XENSTORE v1 03/10] xenstore: check formats of trace Norbert Manthey
@ 2021-02-26 14:41 ` Norbert Manthey
2021-03-02 5:08 ` Jürgen Groß
2021-03-03 16:10 ` Ian Jackson
2021-02-26 14:41 ` [PATCH XENSTORE v1 05/10] xenstore: handle daemon creation errors Norbert Manthey
` (7 subsequent siblings)
11 siblings, 2 replies; 41+ messages in thread
From: Norbert Manthey @ 2021-02-26 14:41 UTC (permalink / raw)
To: xen-devel
Cc: Ian Jackson, Juergen Gross, Wei Liu, Julien Grall, Michael Kurth,
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
^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH XENSTORE v1 05/10] xenstore: handle daemon creation errors
2021-02-26 14:41 [PATCH XENSTORE v1 00/10] Code analysis fixes Norbert Manthey
` (3 preceding siblings ...)
2021-02-26 14:41 ` [PATCH XENSTORE v1 04/10] xenstore_client: handle memory on error Norbert Manthey
@ 2021-02-26 14:41 ` Norbert Manthey
2021-03-02 5:08 ` Jürgen Groß
2021-03-03 16:10 ` Ian Jackson
2021-02-26 14:41 ` [PATCH XENSTORE v1 06/10] xenstored: handle port reads correctly Norbert Manthey
` (6 subsequent siblings)
11 siblings, 2 replies; 41+ messages in thread
From: Norbert Manthey @ 2021-02-26 14:41 UTC (permalink / raw)
To: xen-devel
Cc: Ian Jackson, Juergen Gross, Wei Liu, Julien Grall, Michael Kurth,
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
^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH XENSTORE v1 06/10] xenstored: handle port reads correctly
2021-02-26 14:41 [PATCH XENSTORE v1 00/10] Code analysis fixes Norbert Manthey
` (4 preceding siblings ...)
2021-02-26 14:41 ` [PATCH XENSTORE v1 05/10] xenstore: handle daemon creation errors Norbert Manthey
@ 2021-02-26 14:41 ` Norbert Manthey
2021-02-26 15:36 ` Andrew Cooper
2021-02-26 14:41 ` [PATCH XENSTORE v1 07/10] xenstore: handle do_mkdir and do_rm failure Norbert Manthey
` (5 subsequent siblings)
11 siblings, 1 reply; 41+ messages in thread
From: Norbert Manthey @ 2021-02-26 14:41 UTC (permalink / raw)
To: xen-devel
Cc: Ian Jackson, Juergen Gross, Wei Liu, Julien Grall, Michael Kurth,
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
^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH XENSTORE v1 07/10] xenstore: handle do_mkdir and do_rm failure
2021-02-26 14:41 [PATCH XENSTORE v1 00/10] Code analysis fixes Norbert Manthey
` (5 preceding siblings ...)
2021-02-26 14:41 ` [PATCH XENSTORE v1 06/10] xenstored: handle port reads correctly Norbert Manthey
@ 2021-02-26 14:41 ` Norbert Manthey
2021-03-02 5:09 ` Jürgen Groß
2021-03-03 16:11 ` Ian Jackson
2021-02-26 14:41 ` [PATCH XENSTORE v1 08/10] xenstore: add missing NULL check Norbert Manthey
` (4 subsequent siblings)
11 siblings, 2 replies; 41+ messages in thread
From: Norbert Manthey @ 2021-02-26 14:41 UTC (permalink / raw)
To: xen-devel
Cc: Ian Jackson, Juergen Gross, Wei Liu, Julien Grall, Michael Kurth,
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
^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH XENSTORE v1 08/10] xenstore: add missing NULL check
2021-02-26 14:41 [PATCH XENSTORE v1 00/10] Code analysis fixes Norbert Manthey
` (6 preceding siblings ...)
2021-02-26 14:41 ` [PATCH XENSTORE v1 07/10] xenstore: handle do_mkdir and do_rm failure Norbert Manthey
@ 2021-02-26 14:41 ` Norbert Manthey
2021-03-02 5:10 ` Jürgen Groß
2021-03-03 16:11 ` Ian Jackson
2021-02-26 14:41 ` [PATCH XENSTORE v1 09/10] xs: handle daemon socket error Norbert Manthey
` (3 subsequent siblings)
11 siblings, 2 replies; 41+ messages in thread
From: Norbert Manthey @ 2021-02-26 14:41 UTC (permalink / raw)
To: xen-devel
Cc: Ian Jackson, Juergen Gross, Wei Liu, Julien Grall, Michael Kurth,
Michael Kurth, 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
^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH XENSTORE v1 09/10] xs: handle daemon socket error
2021-02-26 14:41 [PATCH XENSTORE v1 00/10] Code analysis fixes Norbert Manthey
` (7 preceding siblings ...)
2021-02-26 14:41 ` [PATCH XENSTORE v1 08/10] xenstore: add missing NULL check Norbert Manthey
@ 2021-02-26 14:41 ` Norbert Manthey
2021-03-02 5:10 ` Jürgen Groß
2021-03-03 16:13 ` Ian Jackson
2021-02-26 14:41 ` [PATCH XENSTORE v1 10/10] xs: add error handling Norbert Manthey
` (2 subsequent siblings)
11 siblings, 2 replies; 41+ messages in thread
From: Norbert Manthey @ 2021-02-26 14:41 UTC (permalink / raw)
To: xen-devel
Cc: Ian Jackson, Juergen Gross, Wei Liu, Julien Grall, Michael Kurth,
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
^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH XENSTORE v1 10/10] xs: add error handling
2021-02-26 14:41 [PATCH XENSTORE v1 00/10] Code analysis fixes Norbert Manthey
` (8 preceding siblings ...)
2021-02-26 14:41 ` [PATCH XENSTORE v1 09/10] xs: handle daemon socket error Norbert Manthey
@ 2021-02-26 14:41 ` Norbert Manthey
2021-02-26 14:53 ` Julien Grall
` (2 more replies)
2021-03-01 18:01 ` [for-4.15] Re: [PATCH XENSTORE v1 00/10] Code analysis fixes Julien Grall
2021-03-03 18:45 ` Julien Grall
11 siblings, 3 replies; 41+ messages in thread
From: Norbert Manthey @ 2021-02-26 14:41 UTC (permalink / raw)
To: xen-devel
Cc: Ian Jackson, Juergen Gross, Wei Liu, Julien Grall, Michael Kurth,
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
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH XENSTORE v1 10/10] xs: add error handling
2021-02-26 14:41 ` [PATCH XENSTORE v1 10/10] xs: add error handling Norbert Manthey
@ 2021-02-26 14:53 ` Julien Grall
2021-02-26 15:26 ` Norbert Manthey
2021-03-02 5:11 ` Jürgen Groß
2021-03-03 16:13 ` Ian Jackson
2 siblings, 1 reply; 41+ messages in thread
From: Julien Grall @ 2021-02-26 14:53 UTC (permalink / raw)
To: Norbert Manthey, xen-devel
Cc: Ian Jackson, Juergen Gross, Wei Liu, Julien Grall, Michael Kurth
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
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH XENSTORE v1 10/10] xs: add error handling
2021-02-26 14:53 ` Julien Grall
@ 2021-02-26 15:26 ` Norbert Manthey
0 siblings, 0 replies; 41+ messages in thread
From: Norbert Manthey @ 2021-02-26 15:26 UTC (permalink / raw)
To: Julien Grall, xen-devel
Cc: Ian Jackson, Juergen Gross, Wei Liu, Julien Grall, Michael Kurth
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
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH XENSTORE v1 06/10] xenstored: handle port reads correctly
2021-02-26 14:41 ` [PATCH XENSTORE v1 06/10] xenstored: handle port reads correctly Norbert Manthey
@ 2021-02-26 15:36 ` Andrew Cooper
2021-03-02 5:15 ` Jürgen Groß
0 siblings, 1 reply; 41+ messages in thread
From: Andrew Cooper @ 2021-02-26 15:36 UTC (permalink / raw)
To: Norbert Manthey, xen-devel
Cc: Ian Jackson, Juergen Gross, Wei Liu, Julien Grall, Michael Kurth
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
^ permalink raw reply [flat|nested] 41+ messages in thread
* [for-4.15] Re: [PATCH XENSTORE v1 00/10] Code analysis fixes
2021-02-26 14:41 [PATCH XENSTORE v1 00/10] Code analysis fixes Norbert Manthey
` (9 preceding siblings ...)
2021-02-26 14:41 ` [PATCH XENSTORE v1 10/10] xs: add error handling Norbert Manthey
@ 2021-03-01 18:01 ` Julien Grall
2021-03-01 19:39 ` Andrew Cooper
2021-03-03 18:45 ` Julien Grall
11 siblings, 1 reply; 41+ messages in thread
From: Julien Grall @ 2021-03-01 18:01 UTC (permalink / raw)
To: Norbert Manthey, xen-devel
Cc: Ian Jackson, Juergen Gross, Wei Liu, Julien Grall, Michael Kurth
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
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [for-4.15] Re: [PATCH XENSTORE v1 00/10] Code analysis fixes
2021-03-01 18:01 ` [for-4.15] Re: [PATCH XENSTORE v1 00/10] Code analysis fixes Julien Grall
@ 2021-03-01 19:39 ` Andrew Cooper
2021-03-02 10:04 ` Julien Grall
0 siblings, 1 reply; 41+ messages in thread
From: Andrew Cooper @ 2021-03-01 19:39 UTC (permalink / raw)
To: Julien Grall, Norbert Manthey, xen-devel
Cc: Ian Jackson, Juergen Gross, Wei Liu, Julien Grall, Michael Kurth
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
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH XENSTORE v1 01/10] xenstore: add missing NULL check
2021-02-26 14:41 ` [PATCH XENSTORE v1 01/10] xenstore: add missing NULL check Norbert Manthey
@ 2021-03-02 5:07 ` Jürgen Groß
2021-03-03 16:07 ` Ian Jackson
1 sibling, 0 replies; 41+ messages in thread
From: Jürgen Groß @ 2021-03-02 5:07 UTC (permalink / raw)
To: Norbert Manthey, xen-devel
Cc: Ian Jackson, Wei Liu, Julien Grall, Michael Kurth
[-- 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 --]
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH XENSTORE v1 02/10] xenstore: fix print format string
2021-02-26 14:41 ` [PATCH XENSTORE v1 02/10] xenstore: fix print format string Norbert Manthey
@ 2021-03-02 5:07 ` Jürgen Groß
2021-03-03 16:08 ` Ian Jackson
1 sibling, 0 replies; 41+ messages in thread
From: Jürgen Groß @ 2021-03-02 5:07 UTC (permalink / raw)
To: Norbert Manthey, xen-devel
Cc: Ian Jackson, Wei Liu, Julien Grall, Michael Kurth
[-- 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 --]
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH XENSTORE v1 03/10] xenstore: check formats of trace
2021-02-26 14:41 ` [PATCH XENSTORE v1 03/10] xenstore: check formats of trace Norbert Manthey
@ 2021-03-02 5:08 ` Jürgen Groß
2021-03-03 16:08 ` Ian Jackson
1 sibling, 0 replies; 41+ messages in thread
From: Jürgen Groß @ 2021-03-02 5:08 UTC (permalink / raw)
To: Norbert Manthey, xen-devel
Cc: Ian Jackson, Wei Liu, Julien Grall, Michael Kurth
[-- 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 --]
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH XENSTORE v1 04/10] xenstore_client: handle memory on error
2021-02-26 14:41 ` [PATCH XENSTORE v1 04/10] xenstore_client: handle memory on error Norbert Manthey
@ 2021-03-02 5:08 ` Jürgen Groß
2021-03-03 16:10 ` Ian Jackson
1 sibling, 0 replies; 41+ messages in thread
From: Jürgen Groß @ 2021-03-02 5:08 UTC (permalink / raw)
To: Norbert Manthey, xen-devel
Cc: Ian Jackson, Wei Liu, Julien Grall, Michael Kurth
[-- 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 --]
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH XENSTORE v1 05/10] xenstore: handle daemon creation errors
2021-02-26 14:41 ` [PATCH XENSTORE v1 05/10] xenstore: handle daemon creation errors Norbert Manthey
@ 2021-03-02 5:08 ` Jürgen Groß
2021-03-03 16:10 ` Ian Jackson
1 sibling, 0 replies; 41+ messages in thread
From: Jürgen Groß @ 2021-03-02 5:08 UTC (permalink / raw)
To: Norbert Manthey, xen-devel
Cc: Ian Jackson, Wei Liu, Julien Grall, Michael Kurth
[-- 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 --]
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH XENSTORE v1 07/10] xenstore: handle do_mkdir and do_rm failure
2021-02-26 14:41 ` [PATCH XENSTORE v1 07/10] xenstore: handle do_mkdir and do_rm failure Norbert Manthey
@ 2021-03-02 5:09 ` Jürgen Groß
2021-03-03 16:11 ` Ian Jackson
1 sibling, 0 replies; 41+ messages in thread
From: Jürgen Groß @ 2021-03-02 5:09 UTC (permalink / raw)
To: Norbert Manthey, xen-devel
Cc: Ian Jackson, Wei Liu, Julien Grall, Michael Kurth
[-- 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 --]
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH XENSTORE v1 08/10] xenstore: add missing NULL check
2021-02-26 14:41 ` [PATCH XENSTORE v1 08/10] xenstore: add missing NULL check Norbert Manthey
@ 2021-03-02 5:10 ` Jürgen Groß
2021-03-03 16:11 ` Ian Jackson
1 sibling, 0 replies; 41+ messages in thread
From: Jürgen Groß @ 2021-03-02 5:10 UTC (permalink / raw)
To: Norbert Manthey, xen-devel
Cc: Ian Jackson, Wei Liu, Julien Grall, Michael Kurth, Michael Kurth
[-- 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 --]
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH XENSTORE v1 09/10] xs: handle daemon socket error
2021-02-26 14:41 ` [PATCH XENSTORE v1 09/10] xs: handle daemon socket error Norbert Manthey
@ 2021-03-02 5:10 ` Jürgen Groß
2021-03-03 16:13 ` Ian Jackson
1 sibling, 0 replies; 41+ messages in thread
From: Jürgen Groß @ 2021-03-02 5:10 UTC (permalink / raw)
To: Norbert Manthey, xen-devel
Cc: Ian Jackson, Wei Liu, Julien Grall, Michael Kurth
[-- 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 --]
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH XENSTORE v1 10/10] xs: add error handling
2021-02-26 14:41 ` [PATCH XENSTORE v1 10/10] xs: add error handling Norbert Manthey
2021-02-26 14:53 ` Julien Grall
@ 2021-03-02 5:11 ` Jürgen Groß
2021-03-03 16:13 ` Ian Jackson
2 siblings, 0 replies; 41+ messages in thread
From: Jürgen Groß @ 2021-03-02 5:11 UTC (permalink / raw)
To: Norbert Manthey, xen-devel
Cc: Ian Jackson, Wei Liu, Julien Grall, Michael Kurth
[-- 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 --]
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH XENSTORE v1 06/10] xenstored: handle port reads correctly
2021-02-26 15:36 ` Andrew Cooper
@ 2021-03-02 5:15 ` Jürgen Groß
2021-03-02 7:48 ` Norbert Manthey
0 siblings, 1 reply; 41+ messages in thread
From: Jürgen Groß @ 2021-03-02 5:15 UTC (permalink / raw)
To: Andrew Cooper, Norbert Manthey, xen-devel
Cc: Ian Jackson, Wei Liu, Julien Grall, Michael Kurth
[-- 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 --]
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH XENSTORE v1 06/10] xenstored: handle port reads correctly
2021-03-02 5:15 ` Jürgen Groß
@ 2021-03-02 7:48 ` Norbert Manthey
2021-03-02 8:12 ` Jürgen Groß
0 siblings, 1 reply; 41+ messages in thread
From: Norbert Manthey @ 2021-03-02 7:48 UTC (permalink / raw)
To: Jürgen Groß, Andrew Cooper, xen-devel
Cc: Ian Jackson, Wei Liu, Julien Grall, Michael Kurth
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
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH XENSTORE v1 06/10] xenstored: handle port reads correctly
2021-03-02 7:48 ` Norbert Manthey
@ 2021-03-02 8:12 ` Jürgen Groß
0 siblings, 0 replies; 41+ messages in thread
From: Jürgen Groß @ 2021-03-02 8:12 UTC (permalink / raw)
To: Norbert Manthey, Andrew Cooper, xen-devel
Cc: Ian Jackson, Wei Liu, Julien Grall, Michael Kurth
[-- 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 --]
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [for-4.15] Re: [PATCH XENSTORE v1 00/10] Code analysis fixes
2021-03-01 19:39 ` Andrew Cooper
@ 2021-03-02 10:04 ` Julien Grall
0 siblings, 0 replies; 41+ messages in thread
From: Julien Grall @ 2021-03-02 10:04 UTC (permalink / raw)
To: Andrew Cooper, Norbert Manthey, xen-devel
Cc: Ian Jackson, Juergen Gross, Wei Liu, Julien Grall, Michael Kurth
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
^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH XENSTORE v1 01/10] xenstore: add missing NULL check
2021-02-26 14:41 ` [PATCH XENSTORE v1 01/10] xenstore: add missing NULL check Norbert Manthey
2021-03-02 5:07 ` Jürgen Groß
@ 2021-03-03 16:07 ` Ian Jackson
1 sibling, 0 replies; 41+ messages in thread
From: Ian Jackson @ 2021-03-03 16:07 UTC (permalink / raw)
To: Norbert Manthey
Cc: xen-devel, Juergen Gross, Wei Liu, Julien Grall, Michael Kurth
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>
^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH XENSTORE v1 02/10] xenstore: fix print format string
2021-02-26 14:41 ` [PATCH XENSTORE v1 02/10] xenstore: fix print format string Norbert Manthey
2021-03-02 5:07 ` Jürgen Groß
@ 2021-03-03 16:08 ` Ian Jackson
1 sibling, 0 replies; 41+ messages in thread
From: Ian Jackson @ 2021-03-03 16:08 UTC (permalink / raw)
To: Norbert Manthey
Cc: xen-devel, Juergen Gross, Wei Liu, Julien Grall, Michael Kurth
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>
^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH XENSTORE v1 03/10] xenstore: check formats of trace
2021-02-26 14:41 ` [PATCH XENSTORE v1 03/10] xenstore: check formats of trace Norbert Manthey
2021-03-02 5:08 ` Jürgen Groß
@ 2021-03-03 16:08 ` Ian Jackson
1 sibling, 0 replies; 41+ messages in thread
From: Ian Jackson @ 2021-03-03 16:08 UTC (permalink / raw)
To: Norbert Manthey
Cc: xen-devel, Juergen Gross, Wei Liu, Julien Grall, Michael Kurth
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>
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH XENSTORE v1 04/10] xenstore_client: handle memory on error
2021-02-26 14:41 ` [PATCH XENSTORE v1 04/10] xenstore_client: handle memory on error Norbert Manthey
2021-03-02 5:08 ` Jürgen Groß
@ 2021-03-03 16:10 ` Ian Jackson
1 sibling, 0 replies; 41+ messages in thread
From: Ian Jackson @ 2021-03-03 16:10 UTC (permalink / raw)
To: Norbert Manthey
Cc: xen-devel, Juergen Gross, Wei Liu, Julien Grall, Michael Kurth
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.
^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH XENSTORE v1 05/10] xenstore: handle daemon creation errors
2021-02-26 14:41 ` [PATCH XENSTORE v1 05/10] xenstore: handle daemon creation errors Norbert Manthey
2021-03-02 5:08 ` Jürgen Groß
@ 2021-03-03 16:10 ` Ian Jackson
1 sibling, 0 replies; 41+ messages in thread
From: Ian Jackson @ 2021-03-03 16:10 UTC (permalink / raw)
To: Norbert Manthey
Cc: xen-devel, Juergen Gross, Wei Liu, Julien Grall, Michael Kurth
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.
^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH XENSTORE v1 07/10] xenstore: handle do_mkdir and do_rm failure
2021-02-26 14:41 ` [PATCH XENSTORE v1 07/10] xenstore: handle do_mkdir and do_rm failure Norbert Manthey
2021-03-02 5:09 ` Jürgen Groß
@ 2021-03-03 16:11 ` Ian Jackson
1 sibling, 0 replies; 41+ messages in thread
From: Ian Jackson @ 2021-03-03 16:11 UTC (permalink / raw)
To: Norbert Manthey
Cc: xen-devel, Juergen Gross, Wei Liu, Julien Grall, Michael Kurth
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>
^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH XENSTORE v1 08/10] xenstore: add missing NULL check
2021-02-26 14:41 ` [PATCH XENSTORE v1 08/10] xenstore: add missing NULL check Norbert Manthey
2021-03-02 5:10 ` Jürgen Groß
@ 2021-03-03 16:11 ` Ian Jackson
1 sibling, 0 replies; 41+ messages in thread
From: Ian Jackson @ 2021-03-03 16:11 UTC (permalink / raw)
To: Norbert Manthey
Cc: xen-devel, Juergen Gross, Wei Liu, Julien Grall, Michael Kurth,
Michael Kurth
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>
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH XENSTORE v1 09/10] xs: handle daemon socket error
2021-02-26 14:41 ` [PATCH XENSTORE v1 09/10] xs: handle daemon socket error Norbert Manthey
2021-03-02 5:10 ` Jürgen Groß
@ 2021-03-03 16:13 ` Ian Jackson
2021-03-04 14:58 ` Norbert Manthey
1 sibling, 1 reply; 41+ messages in thread
From: Ian Jackson @ 2021-03-03 16:13 UTC (permalink / raw)
To: Norbert Manthey
Cc: xen-devel, Juergen Gross, Wei Liu, Julien Grall, Michael Kurth
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.
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH XENSTORE v1 10/10] xs: add error handling
2021-02-26 14:41 ` [PATCH XENSTORE v1 10/10] xs: add error handling Norbert Manthey
2021-02-26 14:53 ` Julien Grall
2021-03-02 5:11 ` Jürgen Groß
@ 2021-03-03 16:13 ` Ian Jackson
2 siblings, 0 replies; 41+ messages in thread
From: Ian Jackson @ 2021-03-03 16:13 UTC (permalink / raw)
To: Norbert Manthey
Cc: xen-devel, Juergen Gross, Wei Liu, Julien Grall, Michael Kurth
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>
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH XENSTORE v1 00/10] Code analysis fixes
2021-02-26 14:41 [PATCH XENSTORE v1 00/10] Code analysis fixes Norbert Manthey
` (10 preceding siblings ...)
2021-03-01 18:01 ` [for-4.15] Re: [PATCH XENSTORE v1 00/10] Code analysis fixes Julien Grall
@ 2021-03-03 18:45 ` Julien Grall
11 siblings, 0 replies; 41+ messages in thread
From: Julien Grall @ 2021-03-03 18:45 UTC (permalink / raw)
To: Norbert Manthey, xen-devel
Cc: Ian Jackson, Juergen Gross, Wei Liu, Julien Grall, Michael Kurth
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
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH XENSTORE v1 09/10] xs: handle daemon socket error
2021-03-03 16:13 ` Ian Jackson
@ 2021-03-04 14:58 ` Norbert Manthey
2021-03-04 15:04 ` Ian Jackson
0 siblings, 1 reply; 41+ messages in thread
From: Norbert Manthey @ 2021-03-04 14:58 UTC (permalink / raw)
To: Ian Jackson
Cc: xen-devel, Juergen Gross, Wei Liu, Julien Grall, Michael Kurth
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
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH XENSTORE v1 09/10] xs: handle daemon socket error
2021-03-04 14:58 ` Norbert Manthey
@ 2021-03-04 15:04 ` Ian Jackson
0 siblings, 0 replies; 41+ messages in thread
From: Ian Jackson @ 2021-03-04 15:04 UTC (permalink / raw)
To: Norbert Manthey
Cc: xen-devel, Juergen Gross, Wei Liu, Julien Grall, Michael Kurth
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.
^ permalink raw reply [flat|nested] 41+ messages in thread
end of thread, other threads:[~2021-03-04 15:04 UTC | newest]
Thread overview: 41+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-26 14:41 [PATCH XENSTORE v1 00/10] Code analysis fixes Norbert Manthey
2021-02-26 14:41 ` [PATCH XENSTORE v1 01/10] xenstore: add missing NULL check Norbert Manthey
2021-03-02 5:07 ` Jürgen Groß
2021-03-03 16:07 ` Ian Jackson
2021-02-26 14:41 ` [PATCH XENSTORE v1 02/10] xenstore: fix print format string Norbert Manthey
2021-03-02 5:07 ` Jürgen Groß
2021-03-03 16:08 ` Ian Jackson
2021-02-26 14:41 ` [PATCH XENSTORE v1 03/10] xenstore: check formats of trace Norbert Manthey
2021-03-02 5:08 ` Jürgen Groß
2021-03-03 16:08 ` Ian Jackson
2021-02-26 14:41 ` [PATCH XENSTORE v1 04/10] xenstore_client: handle memory on error Norbert Manthey
2021-03-02 5:08 ` Jürgen Groß
2021-03-03 16:10 ` Ian Jackson
2021-02-26 14:41 ` [PATCH XENSTORE v1 05/10] xenstore: handle daemon creation errors Norbert Manthey
2021-03-02 5:08 ` Jürgen Groß
2021-03-03 16:10 ` Ian Jackson
2021-02-26 14:41 ` [PATCH XENSTORE v1 06/10] xenstored: handle port reads correctly Norbert Manthey
2021-02-26 15:36 ` Andrew Cooper
2021-03-02 5:15 ` Jürgen Groß
2021-03-02 7:48 ` Norbert Manthey
2021-03-02 8:12 ` Jürgen Groß
2021-02-26 14:41 ` [PATCH XENSTORE v1 07/10] xenstore: handle do_mkdir and do_rm failure Norbert Manthey
2021-03-02 5:09 ` Jürgen Groß
2021-03-03 16:11 ` Ian Jackson
2021-02-26 14:41 ` [PATCH XENSTORE v1 08/10] xenstore: add missing NULL check Norbert Manthey
2021-03-02 5:10 ` Jürgen Groß
2021-03-03 16:11 ` Ian Jackson
2021-02-26 14:41 ` [PATCH XENSTORE v1 09/10] xs: handle daemon socket error Norbert Manthey
2021-03-02 5:10 ` Jürgen Groß
2021-03-03 16:13 ` Ian Jackson
2021-03-04 14:58 ` Norbert Manthey
2021-03-04 15:04 ` Ian Jackson
2021-02-26 14:41 ` [PATCH XENSTORE v1 10/10] xs: add error handling Norbert Manthey
2021-02-26 14:53 ` Julien Grall
2021-02-26 15:26 ` Norbert Manthey
2021-03-02 5:11 ` Jürgen Groß
2021-03-03 16:13 ` Ian Jackson
2021-03-01 18:01 ` [for-4.15] Re: [PATCH XENSTORE v1 00/10] Code analysis fixes Julien Grall
2021-03-01 19:39 ` Andrew Cooper
2021-03-02 10:04 ` Julien Grall
2021-03-03 18:45 ` Julien Grall
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.