From: Antony Antony <antony@phenome•org>
To: Sabrina Dubroca <sd@queasysnail•net>
Cc: Antony Antony <antony@phenome•org>,
Antony Antony <antony.antony@secunet•com>,
Steffen Klassert <steffen.klassert@secunet•com>,
Herbert Xu <herbert@gondor•apana.org.au>,
netdev@vger•kernel.org, "David S. Miller" <davem@davemloft•net>,
Eric Dumazet <edumazet@google•com>,
Jakub Kicinski <kuba@kernel•org>, Paolo Abeni <pabeni@redhat•com>,
devel@linux-ipsec•org, Leon Romanovsky <leon@kernel•org>,
Eyal Birger <eyal.birger@gmail•com>,
Nicolas Dichtel <nicolas.dichtel@6wind•com>
Subject: Re: [devel-ipsec] [PATCH ipsec-next v10 1/3] xfrm: Add Direction to the SA in or out
Date: Mon, 22 Apr 2024 00:04:55 +0200 [thread overview]
Message-ID: <ZiWNh-Hz9TYWVofO@Antony2201.local> (raw)
In-Reply-To: <Zh44gO885KtSjBHC@hog>
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
next prev parent reply other threads:[~2024-04-21 22:06 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-11 9:40 [PATCH ipsec-next v10 1/3] xfrm: Add Direction to the SA in or out Antony Antony
2024-04-11 9:42 ` [PATCH ipsec-next v10 2/3] xfrm: Add dir validation to "out" data path lookup Antony Antony
2024-04-12 13:49 ` Nicolas Dichtel
2024-04-12 13:53 ` Nicolas Dichtel
2024-04-18 9:24 ` Simon Horman
2024-04-21 22:13 ` Antony Antony
2024-04-11 9:42 ` [PATCH ipsec-next v10 3/3] xfrm: Add dir validation to "in" " Antony Antony
2024-04-12 13:54 ` Nicolas Dichtel
2024-04-15 19:54 ` Antony Antony
2024-04-11 11:41 ` [PATCH ipsec-next v10 1/3] xfrm: Add Direction to the SA in or out Leon Romanovsky
2024-04-11 16:20 ` [devel-ipsec] " Paul Wouters
2024-04-11 16:40 ` Christian Hopps
2024-04-11 17:05 ` Leon Romanovsky
2024-04-15 12:21 ` Sabrina Dubroca
2024-04-16 7:10 ` [devel-ipsec] " Antony Antony
2024-04-16 8:36 ` Sabrina Dubroca
2024-04-21 22:04 ` Antony Antony [this message]
2024-04-22 9:16 ` Sabrina Dubroca
2024-04-22 9:54 ` Nicolas Dichtel
2024-04-23 12:42 ` Antony Antony
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZiWNh-Hz9TYWVofO@Antony2201.local \
--to=antony@phenome$(echo .)org \
--cc=antony.antony@secunet$(echo .)com \
--cc=davem@davemloft$(echo .)net \
--cc=devel@linux-ipsec$(echo .)org \
--cc=edumazet@google$(echo .)com \
--cc=eyal.birger@gmail$(echo .)com \
--cc=herbert@gondor$(echo .)apana.org.au \
--cc=kuba@kernel$(echo .)org \
--cc=leon@kernel$(echo .)org \
--cc=netdev@vger$(echo .)kernel.org \
--cc=nicolas.dichtel@6wind$(echo .)com \
--cc=pabeni@redhat$(echo .)com \
--cc=sd@queasysnail$(echo .)net \
--cc=steffen.klassert@secunet$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox