On 28/07/2023 11:47, Juergen Gross wrote:
On 28.07.23 12:34, Julien Grall wrote:
Because one may want dom0 to send payload bigger than
XENSTORE_PAYLOAD_MAX. Something like:
if ( conn->id != 0 && len < XENSTORE_PAYLOAD_MAX )
Such change would not be caught during compilation. AWS has a similar
check in the downstream tree because the implementation of
non-cooperative migration is using Xenstore command and we want to be
able to transfer the state in a single command.
And how directly is this related to the max data size of node contents?
I think you missed my point. Until this patch, the existing field would
be able to accomodate very large payload. This doesn't hold anymore.
What I was trying to convey is that anyone looking at relaxing the check
in handle_input() needs to be able to find "easily" that other part of
Xenstored are making some assumption based on the maximum length.
As soon as you are allowing dom0 to write larger nodes, you are risking to
kill client connections trying to read such a node. So the node size should
still be limited to XENSTORE_PAYLOAD_MAX.
IMO another reason to use the placement I've suggested.
I agree that BUILD_BUG_ON() makes sense where you suggest if you think
about where the runtime check would happen.
It seems like we have two different aims here. Mine is to make sure we
make it more difficult to introduce a security hole if the lenght check
is relaxed.
I have made a proposal below that may suit both our aim.
AWS should even add
a size check when writing nodes to make sure dom0 doesn't do evil things.
What make you think we don't already have such checked? ;)
Also, I noticed you mention about datalen. What about the number of
permissions?
In case of a runtime check I
agree that a more central place would be preferred.
In the end I don't mind that much, but
BUILD_BUG_ON(XENSTORE_PAYLOAD_MAX >=
(typeof((struct node_hdr *)NULL->datalen))(-1));
is a little bit clumsy IMHO.
Agree. We could introduce FIELD_SIZEOF() (as Linux did) to hide the
complexity. The code would then look like:
>= (8 * FIELD_SIZEOF(struct node_hdr, datalen))
Oh, I guess you mean sizeof_field().
And even with that it would look quite clumsy:
BUILD_BUG_ON(XENSTORE_PAYLOAD_MAX >=
(1UL << (8 * sizeof_field(struct node_hdr, datalen))));
How about keeping the BUILD_BUG_ON() in write_node_raw() and add the
following comment on top of handle_input():
Some fields in Xenstored are sized based on the max payload (see various
BUILD_BUG_ON()). This would need extra runtime check if we ever decide
to have a dynamic payload size.
Cheers,
--
Julien Grall
