| Message ID | 1402935486-29136-3-git-send-email-julien.grall@linaro.org |
|---|---|
| State | Superseded, archived |
| Headers | show |
>>> 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) Both of these ought to be size_t (as it was in the flask original). > +{ > + char *tmp; > + > + if ( size > max_size ) > + return -ENOENT; ENOBUFS would seem the better error code here. > + > + /* 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. And in the end with the above I guess you realize why flask rolled its own special purpose function rather than adding a generic helper. Jan
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,
>>> On 17.06.14 at 11:09, <julien.grall@linaro.org> wrote: > On 17/06/14 09:01, Jan Beulich wrote: >>>>> On 16.06.14 at 18:17, <julien.grall@linaro.org> wrote: >>> + >>> + /* 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. You seem to imply that by not passing in a size I also meant not passing in a maximum size - I didn't say that, though. You absolutely have to limit the string length for security reasons, but it's clearly a difference whether you silently NUL-terminate the value after the maximum number of characters, or return with an error. Jan
On 17/06/14 10:17, Jan Beulich wrote: >>>> On 17.06.14 at 11:09, <julien.grall@linaro.org> wrote: >> On 17/06/14 09:01, Jan Beulich wrote: >>>>>> On 16.06.14 at 18:17, <julien.grall@linaro.org> wrote: >>>> + >>>> + /* 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. > > You seem to imply that by not passing in a size I also meant not > passing in a maximum size - I didn't say that, though. You absolutely > have to limit the string length for security reasons, but it's clearly a > difference whether you silently NUL-terminate the value after the > maximum number of characters, or return with an error. I didn't understand in this way your previous mail. Thank you for the explanation. It looks like for my use case it's better to throw an error if we don't have enough place. It would help us if one day the path start to be very long. I'm wondering if we can also make this change for flask... Daniel, any though? Regards,
On 06/17/2014 05:23 AM, Julien Grall wrote: > > > On 17/06/14 10:17, Jan Beulich wrote: >>>>> On 17.06.14 at 11:09, <julien.grall@linaro.org> wrote: >>> On 17/06/14 09:01, Jan Beulich wrote: >>>>>>> On 16.06.14 at 18:17, <julien.grall@linaro.org> wrote: >>>>> + >>>>> + /* 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. I'm not sure why you would want a string copy-in function to not NUL-terminate the strings it copies in. If you don't want the strings to be NUL-terminated at all, I would call it buffer copy-in function (and copy_from_guest seems to cover buffer copy-in better). If you want the strings to be NUL-terminated and the guest has passed you a length, it's simpler to have the hypervisor add the NUL instead of copying it and then checking that it is there. The current toolstack code for XSM/FLASK relies on the hypervisor to add the NUL terminator, since it often passes in (s, strlen(s)). >>> 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. >> >> You seem to imply that by not passing in a size I also meant not >> passing in a maximum size - I didn't say that, though. You absolutely >> have to limit the string length for security reasons, but it's clearly a >> difference whether you silently NUL-terminate the value after the >> maximum number of characters, or return with an error. > > I didn't understand in this way your previous mail. Thank you for the explanation. > > It looks like for my use case it's better to throw an error if we don't have enough place. It would help us if one day the path start to be very long. > > I'm wondering if we can also make this change for flask... Daniel, any though? Silently cropping the string at some maximum would be a problem for FLASK (and probably other places), as it could result in a valid label that has a different meaning than intended - better to return an error and force the caller to deal with it. Otherwise, I don't think there is any change to be made here, unless I missed something?
>>> On 18.06.14 at 00:43, <dgdegra@tycho.nsa.gov> wrote: > On 06/17/2014 05:23 AM, Julien Grall wrote: >> >> >> On 17/06/14 10:17, Jan Beulich wrote: >>>>>> On 17.06.14 at 11:09, <julien.grall@linaro.org> wrote: >>>> On 17/06/14 09:01, Jan Beulich wrote: >>>>>>>> On 16.06.14 at 18:17, <julien.grall@linaro.org> wrote: >>>>>> + >>>>>> + /* 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. > > I'm not sure why you would want a string copy-in function to not > NUL-terminate the strings it copies in. If you don't want the strings > to be NUL-terminated at all, I would call it buffer copy-in function > (and copy_from_guest seems to cover buffer copy-in better). If you want > the strings to be NUL-terminated and the guest has passed you a length, > it's simpler to have the hypervisor add the NUL instead of copying it > and then checking that it is there. The current toolstack code for > XSM/FLASK relies on the hypervisor to add the NUL terminator, since it > often passes in (s, strlen(s)). I didn't say to just leave such strings unterminated. Instead I said that if there is no zero terminator, rather than putting one there we should just fail the operation if the buffer size limit was exceeded. Jan
On 06/18/2014 12:59 PM, Jan Beulich wrote: >>>> On 18.06.14 at 00:43, <dgdegra@tycho.nsa.gov> wrote: >> On 06/17/2014 05:23 AM, Julien Grall wrote: >>> >>> >>> On 17/06/14 10:17, Jan Beulich wrote: >>>>>>> On 17.06.14 at 11:09, <julien.grall@linaro.org> wrote: >>>>> On 17/06/14 09:01, Jan Beulich wrote: >>>>>>>>> On 16.06.14 at 18:17, <julien.grall@linaro.org> wrote: >>>>>>> + >>>>>>> + /* 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. >> >> I'm not sure why you would want a string copy-in function to not >> NUL-terminate the strings it copies in. If you don't want the strings >> to be NUL-terminated at all, I would call it buffer copy-in function >> (and copy_from_guest seems to cover buffer copy-in better). If you want >> the strings to be NUL-terminated and the guest has passed you a length, >> it's simpler to have the hypervisor add the NUL instead of copying it >> and then checking that it is there. The current toolstack code for >> XSM/FLASK relies on the hypervisor to add the NUL terminator, since it >> often passes in (s, strlen(s)). > > I didn't say to just leave such strings unterminated. Instead I said > that if there is no zero terminator, rather than putting one there we > should just fail the operation if the buffer size limit was exceeded. It looks like I use the same trick as for flask, i.e using strlen(s) and therefore let the hypervisor set the NUL-terminator. I will add a comment on this function to say that we expect the hypervisor to set the NUL-terminator. Regards,
>>> On 18.06.14 at 14:22, <julien.grall@linaro.org> wrote: > On 06/18/2014 12:59 PM, Jan Beulich wrote: >>>>> On 18.06.14 at 00:43, <dgdegra@tycho.nsa.gov> wrote: >>> On 06/17/2014 05:23 AM, Julien Grall wrote: >>>> >>>> >>>> On 17/06/14 10:17, Jan Beulich wrote: >>>>>>>> On 17.06.14 at 11:09, <julien.grall@linaro.org> wrote: >>>>>> On 17/06/14 09:01, Jan Beulich wrote: >>>>>>>>>> On 16.06.14 at 18:17, <julien.grall@linaro.org> wrote: >>>>>>>> + >>>>>>>> + /* 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. >>> >>> I'm not sure why you would want a string copy-in function to not >>> NUL-terminate the strings it copies in. If you don't want the strings >>> to be NUL-terminated at all, I would call it buffer copy-in function >>> (and copy_from_guest seems to cover buffer copy-in better). If you want >>> the strings to be NUL-terminated and the guest has passed you a length, >>> it's simpler to have the hypervisor add the NUL instead of copying it >>> and then checking that it is there. The current toolstack code for >>> XSM/FLASK relies on the hypervisor to add the NUL terminator, since it >>> often passes in (s, strlen(s)). >> >> I didn't say to just leave such strings unterminated. Instead I said >> that if there is no zero terminator, rather than putting one there we >> should just fail the operation if the buffer size limit was exceeded. > > It looks like I use the same trick as for flask, i.e using strlen(s) and > therefore let the hypervisor set the NUL-terminator. > > I will add a comment on this function to say that we expect the > hypervisor to set the NUL-terminator. But just to make sure - the generic helper introduced there shouldn't behave that way if being given the proposed name. Jan
On 06/18/2014 01:49 PM, Jan Beulich wrote: >>>> On 18.06.14 at 14:22, <julien.grall@linaro.org> wrote: >> On 06/18/2014 12:59 PM, Jan Beulich wrote: >>>>>> On 18.06.14 at 00:43, <dgdegra@tycho.nsa.gov> wrote: >>>> On 06/17/2014 05:23 AM, Julien Grall wrote: >>>>> >>>>> >>>>> On 17/06/14 10:17, Jan Beulich wrote: >>>>>>>>> On 17.06.14 at 11:09, <julien.grall@linaro.org> wrote: >>>>>>> On 17/06/14 09:01, Jan Beulich wrote: >>>>>>>>>>> On 16.06.14 at 18:17, <julien.grall@linaro.org> wrote: >>>>>>>>> + >>>>>>>>> + /* 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. >>>> >>>> I'm not sure why you would want a string copy-in function to not >>>> NUL-terminate the strings it copies in. If you don't want the strings >>>> to be NUL-terminated at all, I would call it buffer copy-in function >>>> (and copy_from_guest seems to cover buffer copy-in better). If you want >>>> the strings to be NUL-terminated and the guest has passed you a length, >>>> it's simpler to have the hypervisor add the NUL instead of copying it >>>> and then checking that it is there. The current toolstack code for >>>> XSM/FLASK relies on the hypervisor to add the NUL terminator, since it >>>> often passes in (s, strlen(s)). >>> >>> I didn't say to just leave such strings unterminated. Instead I said >>> that if there is no zero terminator, rather than putting one there we >>> should just fail the operation if the buffer size limit was exceeded. >> >> It looks like I use the same trick as for flask, i.e using strlen(s) and >> therefore let the hypervisor set the NUL-terminator. >> >> I will add a comment on this function to say that we expect the >> hypervisor to set the NUL-terminator. > > But just to make sure - the generic helper introduced there shouldn't > behave that way if being given the proposed name. How will you rename the function?
>>> On 18.06.14 at 14:53, <julien.grall@linaro.org> wrote: > On 06/18/2014 01:49 PM, Jan Beulich wrote: >>>>> On 18.06.14 at 14:22, <julien.grall@linaro.org> wrote: >>> On 06/18/2014 12:59 PM, Jan Beulich wrote: >>>>>>> On 18.06.14 at 00:43, <dgdegra@tycho.nsa.gov> wrote: >>>>> On 06/17/2014 05:23 AM, Julien Grall wrote: >>>>>> >>>>>> >>>>>> On 17/06/14 10:17, Jan Beulich wrote: >>>>>>>>>> On 17.06.14 at 11:09, <julien.grall@linaro.org> wrote: >>>>>>>> On 17/06/14 09:01, Jan Beulich wrote: >>>>>>>>>>>> On 16.06.14 at 18:17, <julien.grall@linaro.org> wrote: >>>>>>>>>> + >>>>>>>>>> + /* 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. >>>>> >>>>> I'm not sure why you would want a string copy-in function to not >>>>> NUL-terminate the strings it copies in. If you don't want the strings >>>>> to be NUL-terminated at all, I would call it buffer copy-in function >>>>> (and copy_from_guest seems to cover buffer copy-in better). If you want >>>>> the strings to be NUL-terminated and the guest has passed you a length, >>>>> it's simpler to have the hypervisor add the NUL instead of copying it >>>>> and then checking that it is there. The current toolstack code for >>>>> XSM/FLASK relies on the hypervisor to add the NUL terminator, since it >>>>> often passes in (s, strlen(s)). >>>> >>>> I didn't say to just leave such strings unterminated. Instead I said >>>> that if there is no zero terminator, rather than putting one there we >>>> should just fail the operation if the buffer size limit was exceeded. >>> >>> It looks like I use the same trick as for flask, i.e using strlen(s) and >>> therefore let the hypervisor set the NUL-terminator. >>> >>> I will add a comment on this function to say that we expect the >>> hypervisor to set the NUL-terminator. >> >> But just to make sure - the generic helper introduced there shouldn't >> behave that way if being given the proposed name. > > How will you rename the function? I don't know. All I know is that the function isn't simply coping in a string. Jan
On 06/18/2014 02:01 PM, Jan Beulich wrote: >> How will you rename the function? > > I don't know. All I know is that the function isn't simply coping in a > string. I though a bit more. I will rename function into safe_copy_string_from_guest and add a comment explain what this function really does (i.e adding a NUL-terminated). Regards,
diff --git a/xen/common/Makefile b/xen/common/Makefile index 3683ae3..627b6e3 100644 --- a/xen/common/Makefile +++ b/xen/common/Makefile @@ -9,6 +9,7 @@ obj-y += event_2l.o obj-y += event_channel.o obj-y += event_fifo.o obj-y += grant_table.o +obj-y += guestcopy.o obj-y += irq.o obj-y += kernel.o obj-y += keyhandler.o diff --git a/xen/common/guestcopy.c b/xen/common/guestcopy.c new file mode 100644 index 0000000..13bde81 --- /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) +{ + char *tmp; + + if ( size > max_size ) + return -ENOENT; + + /* Add an extra +1 to append \0. We can't assume the guest will + * provide a valid string */ + tmp = xmalloc_array(char, size + 1); + if ( !tmp ) + return -ENOMEM; + + if ( copy_from_guest(tmp, u_buf, size) ) + { + xfree(tmp); + return -EFAULT; + } + tmp[size] = 0; + + *buf = tmp; + return 0; +} diff --git a/xen/include/xen/guest_access.h b/xen/include/xen/guest_access.h index 373454e..61756ae 100644 --- a/xen/include/xen/guest_access.h +++ b/xen/include/xen/guest_access.h @@ -8,6 +8,8 @@ #define __XEN_GUEST_ACCESS_H__ #include <asm/guest_access.h> +#include <xen/types.h> +#include <public/xen.h> #define copy_to_guest(hnd, ptr, nr) \ copy_to_guest_offset(hnd, 0, ptr, nr) @@ -27,4 +29,7 @@ #define __clear_guest(hnd, nr) \ __clear_guest_offset(hnd, 0, nr) +int copy_string_from_guest(XEN_GUEST_HANDLE(char) u_buf, char **buf, + unsigned long size, unsigned long max_size); + #endif /* __XEN_GUEST_ACCESS_H__ */ diff --git a/xen/xsm/flask/flask_op.c b/xen/xsm/flask/flask_op.c index 7743aac..6847108 100644 --- a/xen/xsm/flask/flask_op.c +++ b/xen/xsm/flask/flask_op.c @@ -76,29 +76,6 @@ static int domain_has_security(struct domain *d, u32 perms) perms, NULL); } -static int flask_copyin_string(XEN_GUEST_HANDLE(char) u_buf, char **buf, - size_t size, size_t max_size) -{ - char *tmp; - - if ( size > max_size ) - return -ENOENT; - - tmp = xmalloc_array(char, size + 1); - if ( !tmp ) - return -ENOMEM; - - if ( copy_from_guest(tmp, u_buf, size) ) - { - xfree(tmp); - return -EFAULT; - } - tmp[size] = 0; - - *buf = tmp; - return 0; -} - #endif /* COMPAT */ static int flask_security_user(struct xen_flask_userlist *arg) @@ -112,7 +89,7 @@ static int flask_security_user(struct xen_flask_userlist *arg) if ( rv ) return rv; - rv = flask_copyin_string(arg->u.user, &user, arg->size, PAGE_SIZE); + rv = copy_string_from_guest(arg->u.user, &user, arg->size, PAGE_SIZE); if ( rv ) return rv; @@ -227,7 +204,7 @@ static int flask_security_context(struct xen_flask_sid_context *arg) if ( rv ) return rv; - rv = flask_copyin_string(arg->context, &buf, arg->size, PAGE_SIZE); + rv = copy_string_from_guest(arg->context, &buf, arg->size, PAGE_SIZE); if ( rv ) return rv; @@ -324,7 +301,7 @@ static int flask_security_resolve_bool(struct xen_flask_boolean *arg) if ( arg->bool_id != -1 ) return 0; - rv = flask_copyin_string(arg->name, &name, arg->size, bool_maxstr); + rv = copy_string_from_guest(arg->name, &name, arg->size, bool_maxstr); if ( rv ) return rv;
Flask code already provides an helper to copy a string from guest. In a later patch, the new DT hypercalls will need a similar function. To avoid code duplication, copy the flask helper (flask_copying_string) to common code: - Rename into copy_string_from_guest - Update arguments. The size provided by the hypercall is unsigned not signed - Add comment to explain the extra +1 Signed-off-by: Julien Grall <julien.grall@linaro.org> Cc: Daniel De Graaf <dgdegra@tycho.nsa.gov> Cc: Ian Campbell <ian.campbell@citrix.com> Cc: Ian Jackson <ian.jackson@eu.citrix.com> Cc: Jan Beulich <jbeulich@suse.com> Cc: Keir Fraser <keir@xen.org> Cc: Tim Deegan <tim@xen.org> --- xen/common/Makefile | 1 + xen/common/guestcopy.c | 28 ++++++++++++++++++++++++++++ xen/include/xen/guest_access.h | 5 +++++ xen/xsm/flask/flask_op.c | 29 +++-------------------------- 4 files changed, 37 insertions(+), 26 deletions(-) create mode 100644 xen/common/guestcopy.c