[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v3 15/15] argo: validate hypercall arg structures via compat machinery
On Thu, Jan 17, 2019 at 3:25 AM Jan Beulich <JBeulich@xxxxxxxx> wrote: > > >>> On 17.01.19 at 08:22, <christopher.w.clark@xxxxxxxxx> wrote: > > Some details of the problem: > > > > Without the macro overrides in place (ie. using the existing > > definitions) the build fails on CHECK_argo_send_addr because this > > struct is defined with types that are themselves translated by the > > compat processing: > > But that's a normal situation. I thought it would be too but I haven't found a direct equivalent to what this header needs. I'll outline the results of my examination below. > > > typedef struct xen_argo_send_addr > > { > > xen_argo_addr_t src; > > xen_argo_addr_t dst; > > } xen_argo_send_addr_t; > > > > compat/argo.c: In function '__checkFstruct_argo_send_addr__src': > > xen/include/xen/compat.h:170:18: error: comparison of distinct pointer > > types lacks a cast [-Werror] > > return &x->f == &c->f; \ > > ^ > > xen/include/xen/compat.h:176:5: note: in expansion of macro > > 'CHECK_FIELD_COMMON_' > > CHECK_FIELD_COMMON_(k, CHECK_NAME_(k, n ## __ ## f, F), n, f) > > ^~~~~~~~~~~~~~~~~~~ > > xen/include/compat/xlat.h:1238:5: note: in expansion of macro 'CHECK_FIELD_' > > CHECK_FIELD_(struct, argo_send_addr, src); \ > > ^~~~~~~~~~~~ > > compat/argo.c:43:1: note: in expansion of macro 'CHECK_argo_send_addr' > > CHECK_argo_send_addr; > > ^~~~~~~~~~~~~~~~~~~~ > > > > because xen_argo_addr_t is detected as a different type than > > compat_argo_addr_t -- when in practice is the same size and has the > > same fields at the same offsets. > > Did you perhaps not add entries for the inner structures to xlat.lst? No, unfortunately I did. Here are my findings after exploring the compat machinery, in relation to understanding its processing of the the Argo public header file, and its production of the compat header file and validation macros. 1) Two alternative validation macros are possible as output, depending on how struct fields are declared within structs in the input header. Struct fields that are themselves structs can be declared two different ways: * by type (I'll call this "type form"): xen_mystruct_t field; eg. typedef struct xen_argo_send_addr { xen_argo_addr_t src; xen_argo_addr_t dst; } xen_argo_send_addr_t; * by struct name ("struct form"): struct xen_mystruct field; eg. typedef struct xen_argo_send_addr { struct xen_argo_addr src; struct xen_argo_addr dst; } xen_argo_send_addr_t; In the validation macros that are produced for xen_argo_send_addr, the "struct form" contains: CHECK_mystruct; and the "type form" contains instead: CHECK_FIELD_(struct, mystruct, field); These two validation macros do different things; the CHECK_FIELD one is stronger because it tests that the offset, size and type of the field match, whereas the other checks the structure of the inner struct, but not its placement within the outer or matching type in compat vs non-compat. After reviewing other public headers within Xen and the structs in the xlat.lst list, it looks like some existing structs are passing CHECKs by using the "struct" form, even though the checks are weaker. 2. The "type form" checks cannot work for fields that are themselves compat-translated. For fields that are struct types that are themselves compat-translated, the CHECK_FIELD fails because it does a typeof check in the macro, which cannot pass since the non-compat type will never equal the compat-type -- the fields really are different types. So when defining a struct field with a type that will be translated, you have to declare them using the "struct" form, not the "type" form. A prior example of this is: struct mcinfo_extended which declares an array field using the struct form, and not its type. A problem with what the "struct" form generates is that the test that it produces doesn't check the offset of the field within the struct it belongs to. Enabling CHECK_FIELD to work looks preferable as it provides better assurance of correctness. One way to make CHECK_FIELD work is to override the CHECK_COMMON macro, and disable the typeof check when necessary, as has been presented in the versions of the Argo patch series so far. 3. A challenge with using the "struct" form, following from the result of point 2, occurs when it's a XEN_GUEST_HANDLE field within the struct. It's not obvious how to declare that field using the "struct" form rather than the "type" form. This affects the argo_iov struct. 4. Macros to perform "struct form" checks cannot be repeated. When using the "struct" form, it's problem when the struct contains two fields of the same compat-translated type. eg. consider the "struct form" version of xen_argo_send_addr, which has two fields of struct xen_argo_addr: typedef struct xen_argo_send_addr { struct xen_argo_addr src; struct xen_argo_addr dst; } xen_argo_send_addr_t; which then generates this in the compat header: #define CHECK_argo_send_addr \ CHECK_SIZE_(struct, argo_send_addr); \ CHECK_argo_addr; \ CHECK_argo_addr and the second macro invocation of CHECK_argo_addr just breaks, with the build failing due to redefinition of a symbol that is already defined. A (horrible, unacceptable) workaround that unblocks further discovery is to redefine xen_argo_send_addr to use an array: typedef struct xen_argo_send_addr { struct xen_argo_addr addrs[2]; } xen_argo_send_addr_t; which does pass, since it now generates this: #define CHECK_argo_send_addr \ CHECK_SIZE_(struct, argo_send_addr); \ CHECK_argo_addr and then CHECK_argo_send_addr passes, *but* the fields are no longer named as they should be, which is not OK. The "no repeated checks" problem also occurs when another separate struct contains a field of a type that has already been checked: whichever CHECK is performed second will break. eg. typedef struct xen_argo_ring_data_ent { struct xen_argo_addr ring; uint16_t flags; uint16_t pad; uint32_t space_required; uint32_t max_message_size; } xen_argo_ring_data_ent_t; also has a field of type xen_argo_addr, which produces CHECK_argo_addr, which then fails because that was already tested in CHECK_argo_send_addr. Anyway, hopefully this provides context for evaluating the method of passing the compat tests that has been proposed in the Argo series: selective override of the CHECK_FIELD_COMMON macro to disable the typeof checks only for validating those structs that require it to be turned off. > >> > --- a/xen/common/Makefile > >> > +++ b/xen/common/Makefile > >> > @@ -70,7 +70,7 @@ obj-y += xmalloc_tlsf.o > >> > obj-bin-$(CONFIG_X86) += $(foreach n,decompress bunzip2 unxz unlzma > >> > unlzo unlz4 earlycpio,$(n).init.o) > >> > > >> > > >> > -obj-$(CONFIG_COMPAT) += $(addprefix compat/,domain.o kernel.o memory.o > >> > multicall.o xlat.o) > >> > +obj-$(CONFIG_COMPAT) += $(addprefix compat/,argo.o domain.o kernel.o > >> > memory.o multicall.o xlat.o) > >> > >> While a matter of taste to a certain degree, I'm not convinced > >> introducing a separate file for this is really necessary, especially > >> if some of the overrides to the CHECK_* macros would go away. > > > > ack. I wouldn't have moved them out if the overrides weren't in use; > > but I will merge it into the implementation file if that is preferred. > > Well - let's first see whether the overrides are really needed. If so, > keeping this in a separate file might indeed be better. ack. Christopher _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxxxxxxxxx https://lists.xenproject.org/mailman/listinfo/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |