Date: Mon, 22 Apr 2024 00:04:55 +0200
From: Antony Antony <antony@...nome.org>
To: Sabrina Dubroca <sd@...asysnail.net>
Cc: Antony Antony <antony@...nome.org>,
	Antony Antony <antony.antony@...unet.com>,
	Steffen Klassert <steffen.klassert@...unet.com>,
	Herbert Xu <herbert@...dor.apana.org.au>, netdev@...r.kernel.org,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	devel@...ux-ipsec.org, Leon Romanovsky <leon@...nel.org>,
	Eyal Birger <eyal.birger@...il.com>,
	Nicolas Dichtel <nicolas.dichtel@...nd.com>
Subject: Re: [devel-ipsec] [PATCH ipsec-next v10 1/3] xfrm: Add Direction to
 the SA in or out

Hi Sabrina,

On Tue, Apr 16, 2024 at 10:36:16AM +0200, Sabrina Dubroca wrote:
> 2024-04-16, 09:10:25 +0200, Antony Antony wrote:
> > On Mon, Apr 15, 2024 at 02:21:50PM +0200, Sabrina Dubroca via Devel wrote:
> > > 2024-04-11, 11:40:59 +0200, Antony Antony wrote:
> > > > diff --git a/net/xfrm/xfrm_device.c b/net/xfrm/xfrm_device.c
> > > > index 6346690d5c69..2455a76a1cff 100644
> > > > --- a/net/xfrm/xfrm_device.c
> > > > +++ b/net/xfrm/xfrm_device.c
> > > > @@ -253,6 +253,12 @@ int xfrm_dev_state_add(struct net *net, struct xfrm_state *x,
> > > >  		return -EINVAL;
> > > >  	}
> > > > 
> > > > +	if ((xuo->flags & XFRM_OFFLOAD_INBOUND && x->dir == XFRM_SA_DIR_OUT) ||
> > > > +	    (!(xuo->flags & XFRM_OFFLOAD_INBOUND) && x->dir == XFRM_SA_DIR_IN)) {
> > > > +		NL_SET_ERR_MSG(extack, "Mismatched SA and offload direction");
> > > > +		return -EINVAL;
> > > > +	}
> > > 
> > > It would be nice to set x->dir to match the flag, but then I guess the
> > > validation in xfrm_state_update would fail if userspaces tries an
> > > update without providing XFRMA_SA_DIR. (or not because we already went
> > > through this code by the time we get to xfrm_state_update?)
> > 
> > this code already executed from xfrm_state_construct.
> > We could set the in flag in xuo when x->dir == XFRM_SA_DIR_IN, let me think 
> > again.  May be we can do that later:)
> 
> I mean setting x->dir, not setting xuo, ie adding something like this
> to xfrm_dev_state_add:
> 
>     x->dir = xuo->flags & XFRM_OFFLOAD_INBOUND ? XFRM_SA_DIR_IN : XFRM_SA_DIR_OUT;
> 
> xuo will already be set correctly when we're using offload, and won't
> be present if we're not.

Updating with older tools may fail validation. For instance, if a user creates
an SA using an older iproute2 with offload and XFRM_OFFLOAD_INBOUND flag 
set, the kernel sets x->dir = XFRM_SA_DIR_IN. Then, if the user wants to 
update this SA using the same older iproute2, which doesn't allow setting 
dir, the update will fail.

However, as I proposed, if SA dir "in" and offload is enabled, the kernel
could set xuo->flags &= XFRM_OFFLOAD_INBOUND to avoid double typing. I've
considered this, but I'm unsure of any side effects. Also this could be 
added later, which is why I've ignored it for now:)

> > > > diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
> > > > index 810b520493f3..df141edbe8d1 100644
> > > > --- a/net/xfrm/xfrm_user.c
> > > > +++ b/net/xfrm/xfrm_user.c
> > > > +			return -EINVAL;
> > > > +		}
> > > > +
> > > > +		if (x->replay_esn) {
> > > > +			if (x->replay_esn->replay_window > 1) {
> > > > +				NL_SET_ERR_MSG(extack,
> > > > +					       "Replay window should be 1 for OUT SA with ESN");
> > > 
> > > I don't think that we should introduce something we know doesn't make
> > > sense (replay window = 1 on output). It will be API and we won't be
> > > able to fix it up later. We get a chance to make things nice and
> > > reasonable with this new attribute, let's not waste it.
> > >
> > > As I said, AFAICT replay_esn->replay_window isn't used on output, so
> > > unless I missed something, it should just be a matter of changing the
> > > validation. The additional checks in this version should guarantee we
> > > don't have dir==OUT SAs in the packet input path, so this should work.
> > 
> > I agree. Your message and Steffen's message helped me figure out,
> > how to allow replay-window zero for output SA;
> > It is in v11.
> 
> Nice, thanks.
> 
> > > [...]
> > > >  static int xfrm_add_sa(struct sk_buff *skb, struct nlmsghdr *nlh,
> > > >  		       struct nlattr **attrs, struct netlink_ext_ack *extack)
> > > >  {
> > > > @@ -796,6 +881,16 @@ static int xfrm_add_sa(struct sk_buff *skb, struct nlmsghdr *nlh,
> > > >  	if (!x)
> > > >  		return err;
> > > > 
> > > > +	if (x->dir) {
> > > > +		err = verify_sa_dir(x, extack);
> > > > +		if (err) {
> > > > +			x->km.state = XFRM_STATE_DEAD;
> > > > +			xfrm_dev_state_delete(x);
> > > > +			xfrm_state_put(x);
> > > > +			return err;
> > > 
> > > That's not very nice. We're creating a state and just throwing it away
> > > immediately. How hard would it be to validate all that directly from
> > > verify_newsa_info instead?
> > 
> > Your proposal would introduce redundant code, requiring accessing attributes 
> > in verify_newsa_info() and other functions.
> > 
> > The way I propsed, a state x,  xfrm_state, is created but it remains 
> > km.stae=XFRM_STATE_VOID.
> > Newely added verify is before auditing and generating new genid changes, 
> > xfrm_state_add() or xfrm_state_update() would be called later. So deleteing 
> > a state just after xfrm_staet_constructi() is not  bad!
> > 
> > So I think the current code is cleaner, avoiding the need redundant code in 
> > verify_newsa_info().
> 
> Avoids a few easy accesses to the netlink attributes, but allocating a
> state and all its associated info just to throw it away almost
> immediately is not "cleaner" IMO.
> > > And as we discussed, I'd really like XFRMA_SA_DIR to be rejected in
> > > commands that don't use its value.
> > 
> > I still don't see how to add such a check to about 20 functions. A burte 
> > force method would be 18-20 times copy code bellow, with different extack 
> > message.
> 
> Yes, I think with the current netlink infrastructure and a single
> shared policy for all netlink message types, that's what we have to
> do. Doing it in the netlink core (or with help of the netlink core)
> seems difficult, as only the caller (xfrm_user) has all the
> information about which attributes are acceptable with each message
> type.

yes. If we use netlink infrastructure to reject attributes in some methods
shared policy is not ideal. I tried NLA_POLICY_VALIDATE_FN() that function 
does not get message type as an arguent. So add a seperate function in v11.

> 
> > +++ b/net/xfrm/xfrm_user.c
> > @@ -957,6 +957,11 @@ static int xfrm_del_sa(struct sk_buff *skb, struct nlmsghdr *nlh,
> >         struct km_event c;
> >         struct xfrm_usersa_id *p = nlmsg_data(nlh);
> > 
> > +       if (attrs[XFRMA_SA_DIR]) {
> > +               NL_SET_ERR_MSG(extack, "Delete should not have dir attribute set");
> > +               return -ESRCH;
> > +       }
> > +
> > 
> > I am still trying to figure out netlink examples, including the ones you 
> > pointed out : rtnl_valid_dump_net_req, rtnl_net_valid_getid_req.
> 
> These do pretty much what you wrote. 

> 
> > I wonder if there is a way to specifiy rejeced attributes per method.
> >
> > may be there is  way to call nlmsg_parse_deprecated_strict()
> > with .type = NLA_REJECT.
> 
> For that, we'd have to separate the policies for each netlink
> command. Otherwise NLA_REJECT will reject the SA_DIR attribute for all
> commands, which is not what we want.
> 
> > And also this looks like a general cleanup up to me. I wonder how Steffen 
> > would add such a check for the upcoming PCPU attribute! Should that be 
> > prohibited DELSA or XFRM_MSG_FLUSHSA or DELSA?
> 
> IMO, new attributes should be rejected in any handler that doesn't use
> them. That's not a general cleanup because it's a new attribute, and
> the goal is to allow us to decide later if we want to use that
> attribute in DELSA etc. Maybe in one year, we want to make DELSA able
> to match on SA_DIR. If we don't reject SA_DIR from DELSA now, we won't
> be able to do that. That's why I'm insisting on this.

I have implemented a method to reject in v11, even though it is not my 
preference:) My argument xfrm has no precedence of limiting unused 
attributes in most types. We are not enforcing on all attributes such as 
upcoming PCPU. That is why I think it is a general clean up.

I have also tweaked error messages. Removed ESN from it, in v11 verifications
for ESN and non-ESN are similar. Since output SA with ESN allows replay window
0.

-antony

Powered by blists - more mailing lists