Re: [devel-ipsec] [PATCH ipsec-next v10 1/3] xfrm: Add Direction to the SA in or out

[Sabrina Dubroca <sd@queasysnail.net>]

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.


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

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

-- 
Sabrina