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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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@xxxxxxxxxx>
>>> ---
>>> CC: Anthony PERARD <anthony.perard@xxxxxxxxxx>
>>> CC: Juergen Gross <jgross@xxxxxxxx>
>>> ---
>>>   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@xxxxxxx>
>
> With the suggested change:
>
> Reviewed-by: Juergen Gross <jgross@xxxxxxxx>

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

~Andrew