Xen devel - Nov 2013 - Minor Coverity xenstore fixes

Once these and Andrew''s final xenstored have gone in, there should be
no more
xenstore CIDs.

 tools/xenstore/xs.c |   22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

This is for the client-side receiving messages from xenstored, so there
is no security impact, unlike XSA-72.

Coverity-ID: 1055449
Coverity-ID: 1056028
Signed-off-by: Matthew Daley <mattd@bugfuzz.com>
---
 tools/xenstore/xs.c |    6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/tools/xenstore/xs.c b/tools/xenstore/xs.c
index 261b841..184886f 100644
--- a/tools/xenstore/xs.c
+++ b/tools/xenstore/xs.c
@@ -1145,6 +1145,12 @@ static int read_message(struct xs_handle *h, int
nonblocking)
 		goto error_freemsg;
 	}
 
+	/* Sanity check message body length. */
+	if (msg->hdr.len > XENSTORE_PAYLOAD_MAX) {
+		saved_errno = E2BIG;
+		goto error_freemsg;
+	}
+
 	/* Allocate and read the message body. */
 	body = msg->body = malloc(msg->hdr.len + 1);
 	if (body == NULL)
-- 
1.7.10.4

...and check the newly-added result of setnonblock itself where used.

Coverity-ID: 1055103
Signed-off-by: Matthew Daley <mattd@bugfuzz.com>
---
 tools/xenstore/xs.c |   16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/tools/xenstore/xs.c b/tools/xenstore/xs.c
index 184886f..abac788 100644
--- a/tools/xenstore/xs.c
+++ b/tools/xenstore/xs.c
@@ -146,20 +146,17 @@ struct xs_handle {
 
 static int read_message(struct xs_handle *h, int nonblocking);
 
-static void setnonblock(int fd, int nonblock) {
-	int esave = errno;
+static bool setnonblock(int fd, int nonblock) {
 	int flags = fcntl(fd, F_GETFL);
 	if (flags == -1)
-		goto out;
+		return false;
 
 	if (nonblock)
 		flags |= O_NONBLOCK;
 	else
 		flags &= ~O_NONBLOCK;
 
-	fcntl(fd, F_SETFL, flags);
-out:
-	errno = esave;
+	return fcntl(fd, F_SETFL, flags) != -1;
 }
 
 int xs_fileno(struct xs_handle *h)
@@ -369,8 +366,8 @@ static bool read_all(int fd, void *data, unsigned int len,
int nonblocking)
 	if (!len)
 		return true;
 
-	if (nonblocking)
-		setnonblock(fd, 1);
+	if (nonblocking && !setnonblock(fd, 1))
+		return false;
 
 	while (len) {
 		int done;
@@ -390,8 +387,9 @@ static bool read_all(int fd, void *data, unsigned int len,
int nonblocking)
 		len -= done;
 
 		if (nonblocking) {
-			setnonblock(fd, 0);
 			nonblocking = 0;
+			if (!setnonblock(fd, 0))
+				return false;
 		}
 	}
 
-- 
1.7.10.4

...and check the newly-added result of setnonblock itself where used.

Coverity-ID: 1055103
Signed-off-by: Matthew Daley <mattd@bugfuzz.com>
---
v2: return false -> goto out_false

 tools/xenstore/xs.c |   16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/tools/xenstore/xs.c b/tools/xenstore/xs.c
index 184886f..504d524 100644
--- a/tools/xenstore/xs.c
+++ b/tools/xenstore/xs.c
@@ -146,20 +146,17 @@ struct xs_handle {
 
 static int read_message(struct xs_handle *h, int nonblocking);
 
-static void setnonblock(int fd, int nonblock) {
-	int esave = errno;
+static bool setnonblock(int fd, int nonblock) {
 	int flags = fcntl(fd, F_GETFL);
 	if (flags == -1)
-		goto out;
+		return false;
 
 	if (nonblock)
 		flags |= O_NONBLOCK;
 	else
 		flags &= ~O_NONBLOCK;
 
-	fcntl(fd, F_SETFL, flags);
-out:
-	errno = esave;
+	return fcntl(fd, F_SETFL, flags) != -1;
 }
 
 int xs_fileno(struct xs_handle *h)
@@ -369,8 +366,8 @@ static bool read_all(int fd, void *data, unsigned int len,
int nonblocking)
 	if (!len)
 		return true;
 
-	if (nonblocking)
-		setnonblock(fd, 1);
+	if (nonblocking && !setnonblock(fd, 1))
+		return false;
 
 	while (len) {
 		int done;
@@ -390,8 +387,9 @@ static bool read_all(int fd, void *data, unsigned int len,
int nonblocking)
 		len -= done;
 
 		if (nonblocking) {
-			setnonblock(fd, 0);
 			nonblocking = 0;
+			if (!setnonblock(fd, 0))
+				goto out_false;
 		}
 	}
 
-- 
1.7.10.4

On Sat, Nov 30, 2013 at 1:20 PM, Matthew Daley <mattd@bugfuzz.com>
wrote:
> Once these and Andrew''s final xenstored have gone in, there should
be no more
> xenstore CIDs.
Correction: Once these and Andrew''s final xenstored patch have gone
in, there should be no more
> xenstore CIDs in first-party code. There''s still a few in the
tdb/talloc code, which I''m not sure if we just want to fix, or if
it''d be a good idea to look at refreshing our copies from upstream
(Samba).
>
>  tools/xenstore/xs.c |   22 +++++++++++++---------
>  1 file changed, 13 insertions(+), 9 deletions(-)

On 30/11/2013 00:20, Matthew Daley wrote:
> This is for the client-side receiving messages from xenstored, so there
> is no security impact, unlike XSA-72.
>
> Coverity-ID: 1055449
> Coverity-ID: 1056028
> Signed-off-by: Matthew Daley <mattd@bugfuzz.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
>  tools/xenstore/xs.c |    6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/tools/xenstore/xs.c b/tools/xenstore/xs.c
> index 261b841..184886f 100644
> --- a/tools/xenstore/xs.c
> +++ b/tools/xenstore/xs.c
> @@ -1145,6 +1145,12 @@ static int read_message(struct xs_handle *h, int
nonblocking)
>  		goto error_freemsg;
>  	}
>  
> +	/* Sanity check message body length. */
> +	if (msg->hdr.len > XENSTORE_PAYLOAD_MAX) {
> +		saved_errno = E2BIG;
> +		goto error_freemsg;
> +	}
> +
>  	/* Allocate and read the message body. */
>  	body = msg->body = malloc(msg->hdr.len + 1);
>  	if (body == NULL)

On 30/11/2013 00:30, Matthew Daley wrote:
> ...and check the newly-added result of setnonblock itself where used.
>
> Coverity-ID: 1055103
> Signed-off-by: Matthew Daley <mattd@bugfuzz.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> v2: return false -> goto out_false
>
>  tools/xenstore/xs.c |   16 +++++++---------
>  1 file changed, 7 insertions(+), 9 deletions(-)
>
> diff --git a/tools/xenstore/xs.c b/tools/xenstore/xs.c
> index 184886f..504d524 100644
> --- a/tools/xenstore/xs.c
> +++ b/tools/xenstore/xs.c
> @@ -146,20 +146,17 @@ struct xs_handle {
>  
>  static int read_message(struct xs_handle *h, int nonblocking);
>  
> -static void setnonblock(int fd, int nonblock) {
> -	int esave = errno;
> +static bool setnonblock(int fd, int nonblock) {
>  	int flags = fcntl(fd, F_GETFL);
>  	if (flags == -1)
> -		goto out;
> +		return false;
>  
>  	if (nonblock)
>  		flags |= O_NONBLOCK;
>  	else
>  		flags &= ~O_NONBLOCK;
>  
> -	fcntl(fd, F_SETFL, flags);
> -out:
> -	errno = esave;
> +	return fcntl(fd, F_SETFL, flags) != -1;
>  }
>  
>  int xs_fileno(struct xs_handle *h)
> @@ -369,8 +366,8 @@ static bool read_all(int fd, void *data, unsigned int
len, int nonblocking)
>  	if (!len)
>  		return true;
>  
> -	if (nonblocking)
> -		setnonblock(fd, 1);
> +	if (nonblocking && !setnonblock(fd, 1))
> +		return false;
>  
>  	while (len) {
>  		int done;
> @@ -390,8 +387,9 @@ static bool read_all(int fd, void *data, unsigned int
len, int nonblocking)
>  		len -= done;
>  
>  		if (nonblocking) {
> -			setnonblock(fd, 0);
>  			nonblocking = 0;
> +			if (!setnonblock(fd, 0))
> +				goto out_false;
>  		}
>  	}
>

Matthew Daley writes ("[PATCH 1/2] xenstore: sanity check incoming message
body lengths"):
> This is for the client-side receiving messages from xenstored, so there
> is no security impact, unlike XSA-72.
...
> +	/* Sanity check message body length. */
> +	if (msg->hdr.len > XENSTORE_PAYLOAD_MAX) {
> +		saved_errno = E2BIG;
> +		goto error_freemsg;
> +	}
If this situation should arise, your proposal would discard the
headers of the bogus message and read the start of what would be the
over-long payload as the next header.

Unfortunately, it looks like the existing code already does exactly
this if it experiences a malloc failure !

It would be best to either kill the connection dead, or perhaps to
skip the overlong data.

Ian.

Matthew Daley writes ("[PATCH 2/2 v2] xenstore: check F_SETFL fcntl
invocation in setnonblock"):
> ...and check the newly-added result of setnonblock itself where used.
...
> -static void setnonblock(int fd, int nonblock) {
> -	int esave = errno;
> +static bool setnonblock(int fd, int nonblock) {
>  	int flags = fcntl(fd, F_GETFL);
>  	if (flags == -1)
> -		goto out;
> +		return false;
>  
>  	if (nonblock)
>  		flags |= O_NONBLOCK;
>  	else
>  		flags &= ~O_NONBLOCK;
>  
> -	fcntl(fd, F_SETFL, flags);
> -out:
> -	errno = esave;
> +	return fcntl(fd, F_SETFL, flags) != -1;
fcntl F_SETFL returns 0 on success and -1 or error.  But your
setnonblock is supposed to return 1 on success and 0 on error.

Thanks,
Ian.

On Mon, 2013-12-02 at 11:36 +0000, Ian Jackson wrote:
> Matthew Daley writes ("[PATCH 2/2 v2] xenstore: check F_SETFL fcntl
invocation in setnonblock"):
> > ...and check the newly-added result of setnonblock itself where used.
> ...
> > -static void setnonblock(int fd, int nonblock) {
> > -	int esave = errno;
> > +static bool setnonblock(int fd, int nonblock) {
> >  	int flags = fcntl(fd, F_GETFL);
> >  	if (flags == -1)
> > -		goto out;
> > +		return false;
> >  
> >  	if (nonblock)
> >  		flags |= O_NONBLOCK;
> >  	else
> >  		flags &= ~O_NONBLOCK;
> >  
> > -	fcntl(fd, F_SETFL, flags);
> > -out:
> > -	errno = esave;
> > +	return fcntl(fd, F_SETFL, flags) != -1;
> 
> fcntl F_SETFL returns 0 on success and -1 or error.  But your
> setnonblock is supposed to return 1 on success and 0 on error.
The trailing "!= -1" should make the the case, shouldn''t it?

Ian.

On Tue, Dec 3, 2013 at 12:33 AM, Ian Jackson <Ian.Jackson@eu.citrix.com>
wrote:
> Matthew Daley writes ("[PATCH 1/2] xenstore: sanity check incoming
message body lengths"):
>> This is for the client-side receiving messages from xenstored, so there
>> is no security impact, unlike XSA-72.
> ...
>> +     /* Sanity check message body length. */
>> +     if (msg->hdr.len > XENSTORE_PAYLOAD_MAX) {
>> +             saved_errno = E2BIG;
>> +             goto error_freemsg;
>> +     }
>
> If this situation should arise, your proposal would discard the
> headers of the bogus message and read the start of what would be the
> over-long payload as the next header.
>
> Unfortunately, it looks like the existing code already does exactly
> this if it experiences a malloc failure !
>
> It would be best to either kill the connection dead, or perhaps to
> skip the overlong data.
The only callers of read_message I can see are read_reply,
read_watch_internal and read_thread. read_reply''s only caller is
xs_talkv, which closes the connection on the failure being passed
down. read_watch_internal doesn''t, and neither do its callers.
read_thread does close the connection.

So, with that said, where should the handling of the failure go? Would
it be good to consolidate the handling in one spot, ie. directly in
read_message? Since read_message is the root function for all these
other functions, and since read_thread already does handle the failure
and the other functions use that if implicitly if threaded background
reading is enabled, it seems like a good idea to do so.

- Matthew

Ian Campbell writes ("Re: [PATCH 2/2 v2] xenstore: check F_SETFL fcntl
invocation in setnonblock"):
> On Mon, 2013-12-02 at 11:36 +0000, Ian Jackson wrote:
> > > +	return fcntl(fd, F_SETFL, flags) != -1;
> > 
> > fcntl F_SETFL returns 0 on success and -1 or error.  But your
> > setnonblock is supposed to return 1 on success and 0 on error.
> 
> The trailing "!= -1" should make the the case, shouldn''t
it?
Oh, I didn''t spot that.  How horrid.  I think I need to reject this on
style grounds...

Thanks,
Ian.

...and check the newly-added result of setnonblock itself where used.

Coverity-ID: 1055103
Signed-off-by: Matthew Daley <mattd@bugfuzz.com>
---
v2: Use a constant value for the final setnonblock return, instead of a
side-effecting expression

 tools/xenstore/xs.c |   19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/tools/xenstore/xs.c b/tools/xenstore/xs.c
index 184886f..a636498 100644
--- a/tools/xenstore/xs.c
+++ b/tools/xenstore/xs.c
@@ -146,20 +146,20 @@ struct xs_handle {
 
 static int read_message(struct xs_handle *h, int nonblocking);
 
-static void setnonblock(int fd, int nonblock) {
-	int esave = errno;
+static bool setnonblock(int fd, int nonblock) {
 	int flags = fcntl(fd, F_GETFL);
 	if (flags == -1)
-		goto out;
+		return false;
 
 	if (nonblock)
 		flags |= O_NONBLOCK;
 	else
 		flags &= ~O_NONBLOCK;
 
-	fcntl(fd, F_SETFL, flags);
-out:
-	errno = esave;
+	if (fcntl(fd, F_SETFL, flags) == -1)
+		return false;
+
+	return true;
 }
 
 int xs_fileno(struct xs_handle *h)
@@ -369,8 +369,8 @@ static bool read_all(int fd, void *data, unsigned int len,
int nonblocking)
 	if (!len)
 		return true;
 
-	if (nonblocking)
-		setnonblock(fd, 1);
+	if (nonblocking && !setnonblock(fd, 1))
+		return false;
 
 	while (len) {
 		int done;
@@ -390,8 +390,9 @@ static bool read_all(int fd, void *data, unsigned int len,
int nonblocking)
 		len -= done;
 
 		if (nonblocking) {
-			setnonblock(fd, 0);
 			nonblocking = 0;
+			if (!setnonblock(fd, 0))
+				goto out_false;
 		}
 	}
 
-- 
1.7.10.4

Ping?

On Tue, Dec 3, 2013 at 1:45 AM, Matthew Daley <mattd@bugfuzz.com>
wrote:
> ...and check the newly-added result of setnonblock itself where used.
>
> Coverity-ID: 1055103
> Signed-off-by: Matthew Daley <mattd@bugfuzz.com>
> ---
> v2: Use a constant value for the final setnonblock return, instead of a
> side-effecting expression
>
>  tools/xenstore/xs.c |   19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)
>
> diff --git a/tools/xenstore/xs.c b/tools/xenstore/xs.c
> index 184886f..a636498 100644
> --- a/tools/xenstore/xs.c
> +++ b/tools/xenstore/xs.c
> @@ -146,20 +146,20 @@ struct xs_handle {
>
>  static int read_message(struct xs_handle *h, int nonblocking);
>
> -static void setnonblock(int fd, int nonblock) {
> -       int esave = errno;
> +static bool setnonblock(int fd, int nonblock) {
>         int flags = fcntl(fd, F_GETFL);
>         if (flags == -1)
> -               goto out;
> +               return false;
>
>         if (nonblock)
>                 flags |= O_NONBLOCK;
>         else
>                 flags &= ~O_NONBLOCK;
>
> -       fcntl(fd, F_SETFL, flags);
> -out:
> -       errno = esave;
> +       if (fcntl(fd, F_SETFL, flags) == -1)
> +               return false;
> +
> +       return true;
>  }
>
>  int xs_fileno(struct xs_handle *h)
> @@ -369,8 +369,8 @@ static bool read_all(int fd, void *data, unsigned int
len, int nonblocking)
>         if (!len)
>                 return true;
>
> -       if (nonblocking)
> -               setnonblock(fd, 1);
> +       if (nonblocking && !setnonblock(fd, 1))
> +               return false;
>
>         while (len) {
>                 int done;
> @@ -390,8 +390,9 @@ static bool read_all(int fd, void *data, unsigned int
len, int nonblocking)
>                 len -= done;
>
>                 if (nonblocking) {
> -                       setnonblock(fd, 0);
>                         nonblocking = 0;
> +                       if (!setnonblock(fd, 0))
> +                               goto out_false;
>                 }
>         }
>
> --
> 1.7.10.4
>

Matthew Daley writes ("Re: [PATCH 2/2 v2] xenstore: check F_SETFL fcntl
invocation in setnonblock"):
> Ping?
Sorry about that, I have applied this patch (which is actually v3, not
v2).

Regards,
Ian.