[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <ZhhEpNPyKPDohQoH@hog>
Date: Thu, 11 Apr 2024 22:14:28 +0200
From: Sabrina Dubroca <sd@...asysnail.net>
To: Antony Antony <antony@...nome.org>
Cc: Nicolas Dichtel <nicolas.dichtel@...nd.com>,
Antony Antony <antony.antony@...unet.com>,
Herbert Xu <herbert@...dor.apana.org.au>, netdev@...r.kernel.org,
devel@...ux-ipsec.org, Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
"David S. Miller" <davem@...emloft.net>
Subject: Re: [devel-ipsec] [PATCH ipsec-next v6] xfrm: Add Direction to the
SA in or out
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
Powered by blists - more mailing lists