Re: [PATCH v3 17/25] tools/xenstore: rework struct xs_tdb_record_hdr

[Julien Grall <julien@xxxxxxx>]

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