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

[Juergen Gross <jgross@xxxxxxxx>]

On 28.07.23 13:19, Julien Grall wrote:
> 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.
Yes. And the field is related to node data only, so it should be checked to
be large enough where it is written, as this is the critical operation where
truncation would happen.

> 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 the AWS example is showing, the limitation isn't in handle_input().

And TBH: allowing sometimes a payload larger than XENSTORE_PAYLOAD_MAX isn't
something we should encourage. This is rather a very bad design decision.

> > 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.
Correct.

> 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.
Which (as written above) would set a very bad precedence.

XENSTORE_PAYLOAD_MAX should be a hard limit.

> 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? ;)
Just a wild guess. :-)

> Also, I noticed you mention about datalen. What about the number of permissions?
More than max number of domains? Now this would really be crazy. And it would
need operations using more data than XENSTORE_PAYLOAD_MAX allows. :-)

> > > > 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.
I _could_ do that, but where to stop adding such comments?

TBH, I really don't see the point doing that.

In case a patch came up upstream trying to violate XENSTORE_PAYLOAD_MAX I would
surely NACK it.

In case we need payloads larger than XENSTORE_PAYLOAD_MAX we should split the
related operation in multiple parts (see e.g. XS_DIRECTORY_PART or XS_CONTROL
for uploading a new kernel to Xenstore-stubdom for live update). Which is, BTW,
the way AWS should have handled the migration problem (transactions come to my
mind in this context).


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature