1. Patchew
  2. Xen
  3. [PATCH 0/6] tools/libxs: Fix SIGPIPE handling issues
  4. View patch

[PATCH 1/6] tools/libxs: Fix length check in xs_talkv()

Andrew Cooper posted 6 patches 2 months ago
[PATCH 1/6] tools/libxs: Fix length check in xs_talkv()
Posted by Andrew Cooper 2 months ago 
If the sum of iov element lengths overflows, the XENSTORE_PAYLOAD_MAX can
pass, after which we'll write 4G of data with a good-looking length field, and
the remainder of the payload will be interpreted as subsequent commands.

Check each iov element length for XENSTORE_PAYLOAD_MAX before accmulating it.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Anthony PERARD <anthony.perard@vates.tech>
CC: Juergen Gross <jgross@suse.com>
---
 tools/libs/store/xs.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/tools/libs/store/xs.c b/tools/libs/store/xs.c
index ec77379ab9bd..81a790cfe60f 100644
--- a/tools/libs/store/xs.c
+++ b/tools/libs/store/xs.c
@@ -571,21 +571,24 @@ static void *xs_talkv(struct xs_handle *h, xs_transaction_t t,
 	struct xsd_sockmsg msg;
 	void *ret = NULL;
 	int saved_errno;
-	unsigned int i;
+	unsigned int i, msg_len;
 	struct sigaction ignorepipe, oldact;
 
 	msg.tx_id = t;
 	msg.req_id = 0;
 	msg.type = type;
-	msg.len = 0;
-	for (i = 0; i < num_vecs; i++)
-		msg.len += iovec[i].iov_len;
 
-	if (msg.len > XENSTORE_PAYLOAD_MAX) {
-		errno = E2BIG;
-		return 0;
+	/* Calculate the payload length by summing iovec elements */
+	for (i = 0, msg_len = 0; i < num_vecs; i++) {
+		if ((iovec[i].iov_len > XENSTORE_PAYLOAD_MAX) ||
+		    ((msg_len += iovec[i].iov_len) > XENSTORE_PAYLOAD_MAX)) {
+			errno = E2BIG;
+			return 0;
+		}
 	}
 
+	msg.len = msg_len;
+
 	ignorepipe.sa_handler = SIG_IGN;
 	sigemptyset(&ignorepipe.sa_mask);
 	ignorepipe.sa_flags = 0;
-- 
2.39.2
Re: [PATCH 1/6] tools/libxs: Fix length check in xs_talkv()
Posted by Jason Andryuk 1 month, 4 weeks ago 
On 2024-07-18 12:48, Andrew Cooper wrote:
> If the sum of iov element lengths overflows, the XENSTORE_PAYLOAD_MAX can
> pass, after which we'll write 4G of data with a good-looking length field, and
> the remainder of the payload will be interpreted as subsequent commands.
> 
> Check each iov element length for XENSTORE_PAYLOAD_MAX before accmulating it.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Anthony PERARD <anthony.perard@vates.tech>
> CC: Juergen Gross <jgross@suse.com>
> ---
>   tools/libs/store/xs.c | 17 ++++++++++-------
>   1 file changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/tools/libs/store/xs.c b/tools/libs/store/xs.c
> index ec77379ab9bd..81a790cfe60f 100644
> --- a/tools/libs/store/xs.c
> +++ b/tools/libs/store/xs.c
> @@ -571,21 +571,24 @@ static void *xs_talkv(struct xs_handle *h, xs_transaction_t t,
>   	struct xsd_sockmsg msg;
>   	void *ret = NULL;
>   	int saved_errno;
> -	unsigned int i;
> +	unsigned int i, msg_len;
>   	struct sigaction ignorepipe, oldact;
>   
>   	msg.tx_id = t;
>   	msg.req_id = 0;
>   	msg.type = type;
> -	msg.len = 0;
> -	for (i = 0; i < num_vecs; i++)
> -		msg.len += iovec[i].iov_len;
>   
> -	if (msg.len > XENSTORE_PAYLOAD_MAX) {
> -		errno = E2BIG;
> -		return 0;
> +	/* Calculate the payload length by summing iovec elements */
> +	for (i = 0, msg_len = 0; i < num_vecs; i++) {
> +		if ((iovec[i].iov_len > XENSTORE_PAYLOAD_MAX) ||
> +		    ((msg_len += iovec[i].iov_len) > XENSTORE_PAYLOAD_MAX)) {
> +			errno = E2BIG;
> +			return 0;

return NULL;

With that,
Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>
Re: [PATCH 1/6] tools/libxs: Fix length check in xs_talkv()
Posted by Jürgen Groß 1 month, 3 weeks ago 
On 19.07.24 23:14, Jason Andryuk wrote:
> On 2024-07-18 12:48, Andrew Cooper wrote:
>> If the sum of iov element lengths overflows, the XENSTORE_PAYLOAD_MAX can
>> pass, after which we'll write 4G of data with a good-looking length field, and
>> the remainder of the payload will be interpreted as subsequent commands.
>>
>> Check each iov element length for XENSTORE_PAYLOAD_MAX before accmulating it.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> CC: Anthony PERARD <anthony.perard@vates.tech>
>> CC: Juergen Gross <jgross@suse.com>
>> ---
>>   tools/libs/store/xs.c | 17 ++++++++++-------
>>   1 file changed, 10 insertions(+), 7 deletions(-)
>>
>> diff --git a/tools/libs/store/xs.c b/tools/libs/store/xs.c
>> index ec77379ab9bd..81a790cfe60f 100644
>> --- a/tools/libs/store/xs.c
>> +++ b/tools/libs/store/xs.c
>> @@ -571,21 +571,24 @@ static void *xs_talkv(struct xs_handle *h, 
>> xs_transaction_t t,
>>       struct xsd_sockmsg msg;
>>       void *ret = NULL;
>>       int saved_errno;
>> -    unsigned int i;
>> +    unsigned int i, msg_len;
>>       struct sigaction ignorepipe, oldact;
>>       msg.tx_id = t;
>>       msg.req_id = 0;
>>       msg.type = type;
>> -    msg.len = 0;
>> -    for (i = 0; i < num_vecs; i++)
>> -        msg.len += iovec[i].iov_len;
>> -    if (msg.len > XENSTORE_PAYLOAD_MAX) {
>> -        errno = E2BIG;
>> -        return 0;
>> +    /* Calculate the payload length by summing iovec elements */
>> +    for (i = 0, msg_len = 0; i < num_vecs; i++) {
>> +        if ((iovec[i].iov_len > XENSTORE_PAYLOAD_MAX) ||
>> +            ((msg_len += iovec[i].iov_len) > XENSTORE_PAYLOAD_MAX)) {
>> +            errno = E2BIG;
>> +            return 0;
> 
> return NULL;
> 
> With that,
> Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>

With the suggested change:

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


Juergen
Re: [PATCH 1/6] tools/libxs: Fix length check in xs_talkv()
Posted by Andrew Cooper 1 month, 3 weeks ago 
On 22/07/2024 10:19 am, Jürgen Groß wrote:
> On 19.07.24 23:14, Jason Andryuk wrote:
>> On 2024-07-18 12:48, Andrew Cooper wrote:
>>> If the sum of iov element lengths overflows, the
>>> XENSTORE_PAYLOAD_MAX can
>>> pass, after which we'll write 4G of data with a good-looking length
>>> field, and
>>> the remainder of the payload will be interpreted as subsequent
>>> commands.
>>>
>>> Check each iov element length for XENSTORE_PAYLOAD_MAX before
>>> accmulating it.
>>>
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> ---
>>> CC: Anthony PERARD <anthony.perard@vates.tech>
>>> CC: Juergen Gross <jgross@suse.com>
>>> ---
>>>   tools/libs/store/xs.c | 17 ++++++++++-------
>>>   1 file changed, 10 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/tools/libs/store/xs.c b/tools/libs/store/xs.c
>>> index ec77379ab9bd..81a790cfe60f 100644
>>> --- a/tools/libs/store/xs.c
>>> +++ b/tools/libs/store/xs.c
>>> @@ -571,21 +571,24 @@ static void *xs_talkv(struct xs_handle *h,
>>> xs_transaction_t t,
>>>       struct xsd_sockmsg msg;
>>>       void *ret = NULL;
>>>       int saved_errno;
>>> -    unsigned int i;
>>> +    unsigned int i, msg_len;
>>>       struct sigaction ignorepipe, oldact;
>>>       msg.tx_id = t;
>>>       msg.req_id = 0;
>>>       msg.type = type;
>>> -    msg.len = 0;
>>> -    for (i = 0; i < num_vecs; i++)
>>> -        msg.len += iovec[i].iov_len;
>>> -    if (msg.len > XENSTORE_PAYLOAD_MAX) {
>>> -        errno = E2BIG;
>>> -        return 0;
>>> +    /* Calculate the payload length by summing iovec elements */
>>> +    for (i = 0, msg_len = 0; i < num_vecs; i++) {
>>> +        if ((iovec[i].iov_len > XENSTORE_PAYLOAD_MAX) ||
>>> +            ((msg_len += iovec[i].iov_len) > XENSTORE_PAYLOAD_MAX)) {
>>> +            errno = E2BIG;
>>> +            return 0;
>>
>> return NULL;
>>
>> With that,
>> Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>
>
> With the suggested change:
>
> Reviewed-by: Juergen Gross <jgross@suse.com>

Thankyou both.  I'd not even spotted the mistake when just rearranging
the logic.

~Andrew

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.