Re: [PATCH 13/20] tools/xenstore: don't allow creating too many nodes in a transaction

[Juergen Gross <jgross@xxxxxxxx>]

On 06.11.22 23:00, Julien Grall wrote:
> Hi Juergen,
>
> On 01/11/2022 15:28, Juergen Gross wrote:
> > The accounting for the number of nodes of a domain in an active
> > transaction is not working correctly, as it allows to create arbitrary
> > number of nodes. The transaction will finally fail due to exceeding
> > the number of nodes quota, but before closing the transaction an
> > unprivileged guest could cause Xenstore to use a lot of memory.
> I have already made some of comments on the security ML when this was initially 
> set. The arguments still don't sound right to me.
> For a first, since XSA-326, we have a cap in place for the memory a domain can 
> consume. So this surely can't be a "lot of memory". Otherwise we are saying that 
> our limit (there are other way to hit it) were wrong...
Yeah, maybe I should rework the commit message.

The reason I still want to keep the patch is that in a transaction without any
conflicts and without hitting any transaction specific limits (number of nodes
accessed), the errors returned due to a single operation should be the same as
with the same operation performed outside a transaction.

> In addition to that, this is quite pointless to check the number of entry a 
> domain currently owned because this can change before the transaction is 
> committed by another "external" command. Therefore, this would add some 
> randomness in the command which could make more difficult to diagnose.
In the scope of the transaction the tests are correct.

> Lastly, there are other very easy way to use memory (just create multiple 
> transaction in parallel).
> So based on the commit message, the change doesn't help at all to make better 
> Xenstored.
> Note that I don't particularly mind the code change (even though it adds more 
> complexity). I just strongly dislike the current justifications.
> At the moment, I can't find a justification that would make the change 
> whorthwhile.
See above reasoning.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature