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

[Julien Grall <julien.grall@xxxxxxxxxx>]

Hi Jan,

On 17/06/14 09:01, Jan Beulich wrote:
> > > > On 16.06.14 at 18:17, <julien.grall@xxxxxxxxxx> 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

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel