Re: [RFC 02/19] xen: guestcopy: Provide an helper to copy string from guest

[Julien Grall <julien.grall@linaro.org>]

Hi Jan,

On 17/06/14 09:01, Jan Beulich wrote:
>>>> On 16.06.14 at 18:17, <julien.grall@linaro.org> wrote:
>
> While generally I'm okay with adding such a helper, it should be done
> a little more cleanly I think:
>
>> --- /dev/null
>> +++ b/xen/common/guestcopy.c
>> @@ -0,0 +1,28 @@
>> +#include <xen/config.h>
>> +#include <xen/lib.h>
>> +#include <xen/guest_access.h>
>> +
>> +int copy_string_from_guest(XEN_GUEST_HANDLE(char) u_buf, char **buf,
>> +                           unsigned long size, unsigned long max_size)
> 't
> Both of these ought to be size_t (as it was in the flask original).

Hrrmmm... I'm not sure why I made this change. I though the hypercall 
uses unsigned long but it uses uint32_t.

I will use size_t in the next version.



>> +{
>> +    char *tmp;
>> +
>> +    if ( size > max_size )
>> +        return -ENOENT;
>
> ENOBUFS would seem the better error code here.

Ok.

>> +
>> +    /* Add an extra +1 to append \0. We can't assume the guest will
>> +     * provide a valid string */
>
> Now this is the case for flask, but for a generic string copying
> routine I don't think this is desirable. It seems especially wrong to
> aid the guest with putting a NUL where none was. If you really
> want this, I guess you would be better off adding two variants:
> One which demands the string to be NUL-terminated (in which
> case passing in a size is sort of bogus), and one which takes a
> size and inserts a NUL.

A malicious guest could pass a big buffer without a NUL-terminated. If 
we don't limit the size and check the NUL-terminated character the guest 
could respectively exhaust Xen memory and exploit it.

Therefore we can't rely on the guest to provide a valid string. This 
solution will avoid to check in every caller that the string is 
correctly terminated.

Regards,

-- 
Julien Grall