Re: [devel-ipsec] [PATCH ipsec-next v6] xfrm: Add Direction to the SA in or out

[Sabrina Dubroca <sd@queasysnail.net>]

2024-04-11, 12:36:28 +0200, Antony Antony wrote:
> On Thu, Apr 11, 2024 at 11:24:06AM +0200, Sabrina Dubroca wrote:
> > 2024-04-10, 18:59:00 +0200, Antony Antony wrote:
> > > On Wed, Apr 10, 2024 at 10:56:34AM +0200, Sabrina Dubroca wrote:
> > > > 2024-04-09, 19:23:04 +0200, Antony Antony wrote:
> > > > > > xfrma_policy is convenient but not all attributes are valid for all
> > > > > > requests. Old attributes can't be changed, but we should try to be
> > > > > > more strict when we introduce new attributes.
> > > > > 
> > > > > To clarify your feedback, are you suggesting the API should not permit 
> > > > > XFRMA_SA_DIR for methods like XFRM_MSG_DELSA, and only allow it for 
> > > > > XFRM_MSG_NEWSA and XFRM_MSG_UPDSA? I added XFRM_MSG_UPDSA, as it's used 
> > > > > equivalently to XFRM_MSG_NEWSA by *swan.
> > > > 
> > > > Not just DELSA, also all the *POLICY, ALLOCSPI, FLUSHSA, etc. NEWSA
> > > > and UPDSA should accept it, but I'm thinking none of the other
> > > > operations should. It's a property of SAs, not of other xfrm objects.
> > > 
> > > For instance, there isn't a validation for unused XFRMA_SA_EXTRA_FLAGS in 
> > > DELSA; if set, it's simply ignored. Similarly, if XFRMA_SA_DIR were set in 
> > > DELSA, it would also be disregarded. Attempting to introduce validations for 
> > > DELSA and other methods seems like an extensive cleanup task. Do we consider 
> > > this level of validation within the scope of our current patch? It feels 
> > > like we are going too far.
> > 
> > No, I wouldn't introduce validation of other attributes. It doesn't
> > belong in this patch(set), and I'm not sure we can add it now as it
> > might break userspace (I don't see why userspace would pass
> > XFRMA_ALG_AEAD etc on a DELSA request, but if we never rejected it,
> > they could). 
> > 
> > But rejecting this new attribute from messages that don't handle it
> > would be good, and should be done in this patch/series.
> 
> Definitely see the value in such feature in general, but it seems ambitious 
> for this patch set.

I'm only talking about the new attribute here. Introducing validation
for all other attributes, yes, that's a completely separate thing (and
we can't do that immediately, we need to work toward it, see Paul's
suggestion).

> Currently, only NEWSA, UPDSA, and ALLOCSPI need 
> XFRMA_SA_DIR. I am wondering how to reject this atrribute in remaining 20-22 
> messages.  Is there a precedent or example in xfrm_user.c for this kind of 
> validation, or maybe a Netlink feature that lets us restrict NL attributes 
> for a specific messages like DELSA.

I don't think there is, xfrm_user doesn't do that kind of validation yet.
There's an example in rtnl_valid_dump_net_req and
rtnl_net_valid_getid_req, where some attributes are rejected.

-- 
Sabrina