Minor Coverity xenstore fixes

Minor Coverity xenstore fixes

[Matthew Daley]

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(-)

[PATCH 1/2] xenstore: sanity check incoming message body lengths

[Matthew Daley]

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

Re: [PATCH 1/2] xenstore: sanity check incoming message body lengths

[Andrew Cooper]

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)

[PATCH 1/2] xenstore: sanity check incoming message body lengths [and 2 more messages]

[Ian Jackson]

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.

Andrew Cooper writes ("Re: [Xen-devel] [PATCH 1/2] xenstore: sanity check incoming message body lengths"):
> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Matthew Daley writes ("Re: [PATCH 1/2] xenstore: sanity check incoming message body lengths"):
> On Tue, Dec 3, 2013 at 12:33 AM, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote:
> > 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.
...
> > 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.

Yes.

Arguably, based on what you've said, this patch should be applied
as-is, on the grounds of it being an improvement.  Ian C, do you have
an opinion ?

Ian.

Re: [PATCH 1/2] xenstore: sanity check incoming message body lengths [and 2 more messages]

[Ian Campbell]

On Thu, 2013-12-19 at 15:42 +0000, Ian Jackson 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.
> 
> Andrew Cooper writes ("Re: [Xen-devel] [PATCH 1/2] xenstore: sanity check incoming message body lengths"):
> > Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> Matthew Daley writes ("Re: [PATCH 1/2] xenstore: sanity check incoming message body lengths"):
> > On Tue, Dec 3, 2013 at 12:33 AM, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote:
> > > 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.
> ...
> > > 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.
> 
> Yes.
> 
> Arguably, based on what you've said, this patch should be applied
> as-is, on the grounds of it being an improvement.  Ian C, do you have
> an opinion ?

I don't see the harm in taking this, it is an improvement.

Ian.

Re: [PATCH 1/2] xenstore: sanity check incoming message body lengths [and 2 more messages]

[Ian Jackson]

Ian Campbell writes ("Re: [PATCH 1/2] xenstore: sanity check incoming message body lengths [and 2 more messages]"):
> On Thu, 2013-12-19 at 15:42 +0000, Ian Jackson wrote:
> > Arguably, based on what you've said, this patch should be applied
> > as-is, on the grounds of it being an improvement.  Ian C, do you have
> > an opinion ?
> 
> I don't see the harm in taking this, it is an improvement.

Thanks for the opinion.

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
Committed-by: Ian Jackson <ian.jackson@eu.citrix.com>

Also queued for backport.

Ian.

[PATCH 1/2] xenstore: sanity check incoming message body lengths

[Ian Jackson]

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.

Re: [PATCH 1/2] xenstore: sanity check incoming message body lengths

[Matthew Daley]

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

[PATCH 2/2] xenstore: check F_SETFL fcntl invocation in setnonblock

[Matthew Daley]

...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

[PATCH 2/2 v2] xenstore: check F_SETFL fcntl invocation in setnonblock

[Matthew Daley]

...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

Re: [PATCH 2/2 v2] xenstore: check F_SETFL fcntl invocation in setnonblock

[Andrew Cooper]

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

[PATCH 2/2 v2] xenstore: check F_SETFL fcntl invocation in setnonblock

[Ian Jackson]

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.

Re: [PATCH 2/2 v2] xenstore: check F_SETFL fcntl invocation in setnonblock

[Ian Campbell]

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.

Re: [PATCH 2/2 v2] xenstore: check F_SETFL fcntl invocation in setnonblock

[Ian Jackson]

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.

[PATCH 2/2 v2] xenstore: check F_SETFL fcntl invocation in setnonblock

[Matthew Daley]

...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

Re: [PATCH 2/2 v2] xenstore: check F_SETFL fcntl invocation in setnonblock

[Matthew Daley]

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
>

Re: [PATCH 2/2 v2] xenstore: check F_SETFL fcntl invocation in setnonblock

[Ian Jackson]

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.

Re: Minor Coverity xenstore fixes

[Matthew Daley]

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(-)