:p
atchew
Login
While doing the XSA-492 work I further noticed an inefficiency with safe_copy_string_from_guest(). All callers pass PAGE_SIZE as the maximum buffer size, and with the function adding 1 to append a nul terminator the resulting allocations are all order-1 ones. Which we'd better avoid. Require respective callers of hypercalls to nul-terminate the strings within the buffer supplied. While an ABI change, I think it's an acceptable one. 1: libxc: adjust string size calculations in xc_{,de,test_}assign_dt_device() 2: libxc: drop size parameter from xc_flask_context_to_sid() 3: libxc: adjust string size calculations in xc_flask_{getbool_byname,setbool}() 4: lib: make safe_copy_string_from_guest() validate input Jan
In preparation for a hypervisor change also include the nul terminator in the size calculations. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- a/tools/libs/ctrl/xc_domain.c +++ b/tools/libs/ctrl/xc_domain.c @@ -XXX,XX +XXX,XX @@ int xc_assign_dt_device( char *path) { int rc; - size_t size = strlen(path); + size_t size = strlen(path) + 1; struct xen_domctl domctl = {}; DECLARE_HYPERCALL_BOUNCE(path, size, XC_HYPERCALL_BUFFER_BOUNCE_IN); @@ -XXX,XX +XXX,XX @@ int xc_test_assign_dt_device( char *path) { int rc; - size_t size = strlen(path); + size_t size = strlen(path) + 1; struct xen_domctl domctl = {}; DECLARE_HYPERCALL_BOUNCE(path, size, XC_HYPERCALL_BUFFER_BOUNCE_IN); @@ -XXX,XX +XXX,XX @@ int xc_deassign_dt_device( char *path) { int rc; - size_t size = strlen(path); + size_t size = strlen(path) + 1; struct xen_domctl domctl = {}; DECLARE_HYPERCALL_BOUNCE(path, size, XC_HYPERCALL_BUFFER_BOUNCE_IN);
Nul-terminated strings are passed in all cases, so the strlen() can very well be invoked by the function itself. In preparation for a hypervisor change also include the nul terminator in the size calculation. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- Ideally libxl_flask_context_to_sid() would follow suit, but aiui doing so would break its (stable) ABI. Of course the casts in xc_flask_access() are suspicious. --- a/tools/helpers/init-xenstore-domain.c +++ b/tools/helpers/init-xenstore-domain.c @@ -XXX,XX +XXX,XX @@ static int build(xc_interface *xch) if ( flask ) { - rv = xc_flask_context_to_sid(xch, flask, strlen(flask), &config.ssidref); + rv = xc_flask_context_to_sid(xch, flask, &config.ssidref); if ( rv ) { fprintf(stderr, "xc_flask_context_to_sid failed\n"); --- a/tools/include/xenctrl.h +++ b/tools/include/xenctrl.h @@ -XXX,XX +XXX,XX @@ long xc_sharing_used_frames(xc_interface /*** End sharing interface ***/ int xc_flask_load(xc_interface *xc_handle, char *buf, uint32_t size); -int xc_flask_context_to_sid(xc_interface *xc_handle, char *buf, uint32_t size, uint32_t *sid); +int xc_flask_context_to_sid(xc_interface *xc_handle, char *buf, uint32_t *sid); int xc_flask_sid_to_context(xc_interface *xc_handle, int sid, char *buf, uint32_t size); int xc_flask_getenforce(xc_interface *xc_handle); int xc_flask_setenforce(xc_interface *xc_handle, int mode); --- a/tools/libs/ctrl/xc_flask.c +++ b/tools/libs/ctrl/xc_flask.c @@ -XXX,XX +XXX,XX @@ int xc_flask_load(xc_interface *xch, cha return err; } -int xc_flask_context_to_sid(xc_interface *xch, char *buf, uint32_t size, uint32_t *sid) +int xc_flask_context_to_sid(xc_interface *xch, char *buf, uint32_t *sid) { int err; struct xen_flask_op op = {}; + size_t size = strlen(buf) + 1; DECLARE_HYPERCALL_BOUNCE(buf, size, XC_HYPERCALL_BUFFER_BOUNCE_IN); if ( xc_hypercall_bounce_pre(xch, buf) ) @@ -XXX,XX +XXX,XX @@ static int xc_flask_add(xc_interface *xc int err; struct xen_flask_op op = {}; - err = xc_flask_context_to_sid(xch, scontext, strlen(scontext), &sid); + err = xc_flask_context_to_sid(xch, scontext, &sid); if ( err ) return err; @@ -XXX,XX +XXX,XX @@ int xc_flask_access(xc_interface *xch, c struct xen_flask_op op = {}; int err; - err = xc_flask_context_to_sid(xch, (char*)scon, strlen(scon), &op.u.access.ssid); + err = xc_flask_context_to_sid(xch, (char*)scon, &op.u.access.ssid); if ( err ) return err; - err = xc_flask_context_to_sid(xch, (char*)tcon, strlen(tcon), &op.u.access.tsid); + err = xc_flask_context_to_sid(xch, (char*)tcon, &op.u.access.tsid); if ( err ) return err; --- a/tools/libs/light/libxl_flask.c +++ b/tools/libs/light/libxl_flask.c @@ -XXX,XX +XXX,XX @@ int libxl_flask_context_to_sid(libxl_ctx { int rc; - rc = xc_flask_context_to_sid(ctx->xch, buf, len, ssidref); + assert(len == strlen(buf)); + rc = xc_flask_context_to_sid(ctx->xch, buf, ssidref); return rc; } --- a/tools/python/xen/lowlevel/xc/xc.c +++ b/tools/python/xen/lowlevel/xc/xc.c @@ -XXX,XX +XXX,XX @@ static PyObject *pyflask_context_to_sid( return PyErr_SetFromErrno(xc_error_obj); } - ret = xc_flask_context_to_sid(xc_handle, ctx, strlen(ctx), &sid); + ret = xc_flask_context_to_sid(xc_handle, ctx, &sid); xc_interface_close(xc_handle);
In preparation for a hypervisor change also include the nul terminator in the size calculations. (Note that xc_flask_getbool_byid() doesn't support FLASK_GETBOOL's "ID being -1" variant of operation, and hence doesn't need fiddling with. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- a/tools/libs/ctrl/xc_flask.c +++ b/tools/libs/ctrl/xc_flask.c @@ -XXX,XX +XXX,XX @@ int xc_flask_getbool_byname(xc_interface { int rv; struct xen_flask_op op = {}; - DECLARE_HYPERCALL_BOUNCE(name, strlen(name), XC_HYPERCALL_BUFFER_BOUNCE_IN); + size_t size = strlen(name) + 1; + DECLARE_HYPERCALL_BOUNCE(name, size, XC_HYPERCALL_BUFFER_BOUNCE_IN); if ( xc_hypercall_bounce_pre(xch, name) ) { @@ -XXX,XX +XXX,XX @@ int xc_flask_getbool_byname(xc_interface op.cmd = FLASK_GETBOOL; op.u.boolean.bool_id = -1; - op.u.boolean.size = strlen(name); + op.u.boolean.size = size; set_xen_guest_handle(op.u.boolean.name, name); rv = xc_flask_op(xch, &op); @@ -XXX,XX +XXX,XX @@ int xc_flask_setbool(xc_interface *xch, { int rv; struct xen_flask_op op = {}; - DECLARE_HYPERCALL_BOUNCE(name, strlen(name), XC_HYPERCALL_BUFFER_BOUNCE_IN); + size_t size = strlen(name) + 1; + DECLARE_HYPERCALL_BOUNCE(name, size, XC_HYPERCALL_BUFFER_BOUNCE_IN); if ( xc_hypercall_bounce_pre(xch, name) ) { @@ -XXX,XX +XXX,XX @@ int xc_flask_setbool(xc_interface *xch, op.u.boolean.bool_id = -1; op.u.boolean.new_value = value; op.u.boolean.commit = 1; - op.u.boolean.size = strlen(name); + op.u.boolean.size = size; set_xen_guest_handle(op.u.boolean.name, name); rv = xc_flask_op(xch, &op);
... rather than papering over guest flaws: Strings passed ought to be nul- terminated (yet sadly libxc hasn't been doing so thus far). This way we also avoid order-1 allocations, seeing that all present callers pass PAGE_SIZE for max_size. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- I can't spot any caller side use of FLASK_DEVICETREE_LABEL, hence there's no corresponding prereq patch. --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -XXX,XX +XXX,XX @@ The format is based on [Keep a Changelog to obtain an automatically allocated domid. The prior sentinel values (0 since the start of Xen, and DOMID_INVALID since Xen 4.21) now no longer represent a wildcard input. + - XEN_DOMCTL_DEV_DT's, FLASK_[GS]ETBOOL's, and FLASK_DEVICETREE_LABEL's input + string sizes need to include the nul terminator. - On x86: - Enable pf-fixup option by default for PVH dom0. - The libxenguest bzImage loader now uses the system liblz4 library. --- a/xen/lib/guest-strcpy.c +++ b/xen/lib/guest-strcpy.c @@ -XXX,XX +XXX,XX @@ #include <xen/err.h> /* - * The function copies a string from the guest and adds a NUL to - * make sure the string is correctly terminated. + * The function copies a string from the guest and checks there's a NUL + * terminating the string. */ char *safe_copy_string_from_guest(XEN_GUEST_HANDLE(char) u_buf, size_t size, size_t max_size) @@ -XXX,XX +XXX,XX @@ char *safe_copy_string_from_guest(XEN_GU if ( size > max_size ) return ERR_PTR(-ENOBUFS); - /* Add an extra +1 to append \0 */ - tmp = xmalloc_array(char, size + 1); + tmp = xmalloc_array(char, size); if ( !tmp ) return ERR_PTR(-ENOMEM); @@ -XXX,XX +XXX,XX @@ char *safe_copy_string_from_guest(XEN_GU xfree(tmp); return ERR_PTR(-EFAULT); } - tmp[size] = '\0'; + + if ( !memchr(tmp, 0, size) ) + { + xfree(tmp); + return ERR_PTR(-EMSGSIZE); + } return tmp; } --- a/xen/include/public/domctl.h +++ b/xen/include/public/domctl.h @@ -XXX,XX +XXX,XX @@ struct xen_domctl_assign_device { uint32_t machine_sbdf; /* machine PCI ID of assigned device */ } pci; struct { - uint32_t size; /* Length of the path */ + uint32_t size; /* Length of the path, including nul terminator */ XEN_GUEST_HANDLE_64(char) path; /* Path to the device tree node */ #ifdef __XEN__ struct dt_device_node *dev; /* Resolved device node of the above */ --- a/xen/include/public/xsm/flask_op.h +++ b/xen/include/public/xsm/flask_op.h @@ -XXX,XX +XXX,XX @@ typedef struct xen_flask_setenforce xen_ struct xen_flask_sid_context { /* IN/OUT: sid to convert to/from string */ uint32_t sid; - /* IN: size of the context buffer + /* + * IN: size of the context buffer, including nul terminator * OUT: actual size of the output context string */ uint32_t size; @@ -XXX,XX +XXX,XX @@ struct xen_flask_boolean { uint8_t new_value; /* IN: commit new value instead of only setting pending [SET] */ uint8_t commit; - /* IN: size of boolean name buffer [GET/SET] - * OUT: actual size of name [GET only] */ + /* + * IN: size of boolean name buffer [GET/SET]; must cover nul terminator + * if "name" (below) is an input + * OUT: actual size of name [GET only] + */ uint32_t size; /* IN: if bool_id is -1, used to find boolean [GET/SET] * OUT: textual name of boolean [GET only] @@ -XXX,XX +XXX,XX @@ typedef struct xen_flask_relabel xen_fla struct xen_flask_devicetree_label { /* IN */ uint32_t sid; - uint32_t length; + uint32_t length; /* length of the path, including nul terminator */ XEN_GUEST_HANDLE(char) path; }; typedef struct xen_flask_devicetree_label xen_flask_devicetree_label_t;
While doing the XSA-492 work I further noticed an inefficiency with safe_copy_string_from_guest(). All callers pass PAGE_SIZE as the maximum buffer size, and with the function adding 1 to append a nul terminator the resulting allocations are all order-1 ones. Which we'd better avoid. Require respective callers of hypercalls to nul-terminate the strings within the buffer supplied. While an ABI change, I think it's an acceptable one. 1: libxc: drop size parameter from xc_flask_context_to_sid() 2: lib: make safe_copy_string_from_guest() validate input Jan
Nul-terminated strings are passed in all cases, so the strlen() can very well be invoked by the function itself. In preparation for a hypervisor change also include the nul terminator in the size calculation. Signed-off-by: Jan Beulich <jbeulich@suse.com> Acked-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> --- Ideally libxl_flask_context_to_sid() would follow suit, but aiui doing so would break its (stable) API. Of course the casts in xc_flask_access() are suspicious. --- v2: Avoid assert() use in libxl. --- a/tools/helpers/init-xenstore-domain.c +++ b/tools/helpers/init-xenstore-domain.c @@ -XXX,XX +XXX,XX @@ static int build(xc_interface *xch) if ( flask ) { - rv = xc_flask_context_to_sid(xch, flask, strlen(flask), &config.ssidref); + rv = xc_flask_context_to_sid(xch, flask, &config.ssidref); if ( rv ) { fprintf(stderr, "xc_flask_context_to_sid failed\n"); --- a/tools/include/xenctrl.h +++ b/tools/include/xenctrl.h @@ -XXX,XX +XXX,XX @@ long xc_sharing_used_frames(xc_interface /*** End sharing interface ***/ int xc_flask_load(xc_interface *xc_handle, char *buf, uint32_t size); -int xc_flask_context_to_sid(xc_interface *xc_handle, char *buf, uint32_t size, uint32_t *sid); +int xc_flask_context_to_sid(xc_interface *xc_handle, char *buf, uint32_t *sid); int xc_flask_sid_to_context(xc_interface *xc_handle, int sid, char *buf, uint32_t size); int xc_flask_getenforce(xc_interface *xc_handle); int xc_flask_setenforce(xc_interface *xc_handle, int mode); --- a/tools/libs/ctrl/xc_flask.c +++ b/tools/libs/ctrl/xc_flask.c @@ -XXX,XX +XXX,XX @@ int xc_flask_load(xc_interface *xch, cha return err; } -int xc_flask_context_to_sid(xc_interface *xch, char *buf, uint32_t size, uint32_t *sid) +int xc_flask_context_to_sid(xc_interface *xch, char *buf, uint32_t *sid) { int err; struct xen_flask_op op = {}; + size_t size = strlen(buf) + 1; DECLARE_HYPERCALL_BOUNCE(buf, size, XC_HYPERCALL_BUFFER_BOUNCE_IN); if ( xc_hypercall_bounce_pre(xch, buf) ) @@ -XXX,XX +XXX,XX @@ static int xc_flask_add(xc_interface *xc int err; struct xen_flask_op op = {}; - err = xc_flask_context_to_sid(xch, scontext, strlen(scontext), &sid); + err = xc_flask_context_to_sid(xch, scontext, &sid); if ( err ) return err; @@ -XXX,XX +XXX,XX @@ int xc_flask_access(xc_interface *xch, c struct xen_flask_op op = {}; int err; - err = xc_flask_context_to_sid(xch, (char*)scon, strlen(scon), &op.u.access.ssid); + err = xc_flask_context_to_sid(xch, (char*)scon, &op.u.access.ssid); if ( err ) return err; - err = xc_flask_context_to_sid(xch, (char*)tcon, strlen(tcon), &op.u.access.tsid); + err = xc_flask_context_to_sid(xch, (char*)tcon, &op.u.access.tsid); if ( err ) return err; --- a/tools/libs/light/libxl_flask.c +++ b/tools/libs/light/libxl_flask.c @@ -XXX,XX +XXX,XX @@ int libxl_flask_context_to_sid(libxl_ctx { int rc; - rc = xc_flask_context_to_sid(ctx->xch, buf, len, ssidref); + if (len != strlen(buf)) + return ERROR_INVAL; + + rc = xc_flask_context_to_sid(ctx->xch, buf, ssidref); return rc; } --- a/tools/python/xen/lowlevel/xc/xc.c +++ b/tools/python/xen/lowlevel/xc/xc.c @@ -XXX,XX +XXX,XX @@ static PyObject *pyflask_context_to_sid( return PyErr_SetFromErrno(xc_error_obj); } - ret = xc_flask_context_to_sid(xc_handle, ctx, strlen(ctx), &sid); + ret = xc_flask_context_to_sid(xc_handle, ctx, &sid); xc_interface_close(xc_handle);
... rather than papering over guest flaws: Strings passed ought to be nul- terminated (yet sadly libxc hasn't been doing so thus far). This way we also avoid order-1 allocations, seeing that all present callers pass PAGE_SIZE for max_size. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- I can't spot any caller side use of FLASK_DEVICETREE_LABEL, hence there's no corresponding prereq patch. --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -XXX,XX +XXX,XX @@ The format is based on [Keep a Changelog ## [4.23.0 UNRELEASED](https://xenbits.xenproject.org/gitweb/?p=xen.git;a=shortlog;h=staging) - TBD ### Changed + - XEN_DOMCTL_DEV_DT's, FLASK_[GS]ETBOOL's, and FLASK_DEVICETREE_LABEL's input + string sizes need to include the nul terminator. ### Added --- a/xen/lib/guest-strcpy.c +++ b/xen/lib/guest-strcpy.c @@ -XXX,XX +XXX,XX @@ #include <xen/err.h> /* - * The function copies a string from the guest and adds a NUL to - * make sure the string is correctly terminated. + * The function copies a string from the guest and checks there's a NUL + * terminating the string. */ char *safe_copy_string_from_guest(XEN_GUEST_HANDLE(char) u_buf, size_t size, size_t max_size) @@ -XXX,XX +XXX,XX @@ char *safe_copy_string_from_guest(XEN_GU if ( size > max_size ) return ERR_PTR(-ENOBUFS); - /* Add an extra +1 to append \0 */ - tmp = xmalloc_array(char, size + 1); + tmp = xmalloc_array(char, size); if ( !tmp ) return ERR_PTR(-ENOMEM); @@ -XXX,XX +XXX,XX @@ char *safe_copy_string_from_guest(XEN_GU xfree(tmp); return ERR_PTR(-EFAULT); } - tmp[size] = '\0'; + + if ( !memchr(tmp, 0, size) ) + { + xfree(tmp); + return ERR_PTR(-EMSGSIZE); + } return tmp; } --- a/xen/include/public/domctl.h +++ b/xen/include/public/domctl.h @@ -XXX,XX +XXX,XX @@ struct xen_domctl_assign_device { uint32_t machine_sbdf; /* machine PCI ID of assigned device */ } pci; struct { - uint32_t size; /* Length of the path */ + uint32_t size; /* Length of the path, including nul terminator */ XEN_GUEST_HANDLE_64(char) path; /* Path to the device tree node */ #ifdef __XEN__ struct dt_device_node *dev; /* Resolved device node of the above */ --- a/xen/include/public/xsm/flask_op.h +++ b/xen/include/public/xsm/flask_op.h @@ -XXX,XX +XXX,XX @@ typedef struct xen_flask_setenforce xen_ struct xen_flask_sid_context { /* IN/OUT: sid to convert to/from string */ uint32_t sid; - /* IN: size of the context buffer + /* + * IN: size of the context buffer, including nul terminator * OUT: actual size of the output context string */ uint32_t size; @@ -XXX,XX +XXX,XX @@ struct xen_flask_boolean { uint8_t new_value; /* IN: commit new value instead of only setting pending [SET] */ uint8_t commit; - /* IN: size of boolean name buffer [GET/SET] - * OUT: actual size of name [GET only] */ + /* + * IN: size of boolean name buffer [GET/SET]; must cover nul terminator + * if "name" (below) is an input + * OUT: actual size of name [GET only] + */ uint32_t size; /* IN: if bool_id is -1, used to find boolean [GET/SET] * OUT: textual name of boolean [GET only] @@ -XXX,XX +XXX,XX @@ typedef struct xen_flask_relabel xen_fla struct xen_flask_devicetree_label { /* IN */ uint32_t sid; - uint32_t length; + uint32_t length; /* length of the path, including nul terminator */ XEN_GUEST_HANDLE(char) path; }; typedef struct xen_flask_devicetree_label xen_flask_devicetree_label_t;