* xfrm: Fix initialize repl field of struct xfrm_state
@ 2011-03-21 5:45 Wei Yongjun
2011-03-21 5:48 ` [PATCH] " Wei Yongjun
2011-03-21 5:55 ` David Miller
0 siblings, 2 replies; 18+ messages in thread
From: Wei Yongjun @ 2011-03-21 5:45 UTC (permalink / raw)
To: netdev, David Miller, Steffen Klassert
Commit 'xfrm: Move IPsec replay detection functions to a separate file'
(9fdc4883d92d20842c5acea77a4a21bb1574b495)
introduce repl field to struct xfrm_state, and only initialize it
under SA's netlink create path, the other path, such as pf_key, the
repl field remaining uninitialize. So if the SA is created by pf_key,
any input packet with SA's encryption algorithm will cause panic.
int xfrm_input()
{
...
x->repl->advance(x, seq);
...
}
This patch fixed to init default xfrm_replay in xfrm_init_state().
Pid: 0, comm: swapper Not tainted 2.6.38-next+ #14 Bochs Bochs
EIP: 0060:[<c078e5d5>] EFLAGS: 00010206 CPU: 0
EIP is at xfrm_input+0x31c/0x4cc
EAX: dd839c00 EBX: 00000084 ECX: 00000000 EDX: 01000000
ESI: dd839c00 EDI: de3a0780 EBP: dec1de88 ESP: dec1de64
DS: 007b ES: 007b FS: 00d8 GS: 00e0 SS: 0068
Process swapper (pid: 0, ti=dec1c000 task=c09c0f20 task.ti=c0992000)
Stack:
00000000 00000000 00000002 c0ba27c0 00100000 01000000 de3a0798 c0ba27c0
00000033 dec1de98 c0786848 00000000 de3a0780 dec1dea4 c0786868 00000000
dec1debc c074ee56 e1da6b8c de3a0780 c074ed44 de3a07a8 dec1decc c074ef32
Call Trace:
[<c0786848>] xfrm4_rcv_encap+0x22/0x27
[<c0786868>] xfrm4_rcv+0x1b/0x1d
[<c074ee56>] ip_local_deliver_finish+0x112/0x1b1
[<c074ed44>] ? ip_local_deliver_finish+0x0/0x1b1
[<c074ef32>] NF_HOOK.clone.1+0x3d/0x44
[<c074ef77>] ip_local_deliver+0x3e/0x44
[<c074ed44>] ? ip_local_deliver_finish+0x0/0x1b1
[<c074ec03>] ip_rcv_finish+0x30a/0x332
[<c074e8f9>] ? ip_rcv_finish+0x0/0x332
[<c074ef32>] NF_HOOK.clone.1+0x3d/0x44
[<c074f188>] ip_rcv+0x20b/0x247
[<c074e8f9>] ? ip_rcv_finish+0x0/0x332
[<c072797d>] __netif_receive_skb+0x373/0x399
[<c0727bc1>] netif_receive_skb+0x4b/0x51
[<e0817e2a>] cp_rx_poll+0x210/0x2c4 [8139cp]
[<c072818f>] net_rx_action+0x9a/0x17d
[<c0445b5c>] __do_softirq+0xa1/0x149
[<c0445abb>] ? __do_softirq+0x0/0x149
Signed-off-by: Wei Yongjun <yjwei@cn.fujitsu.com>
---
net/xfrm/xfrm_state.c | 4 ++++
1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
index d575f05..4274e11 100644
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c
@@ -1980,6 +1980,10 @@ int xfrm_init_state(struct xfrm_state *x)
if (x->outer_mode == NULL)
goto error;
+ err = xfrm_init_replay(x);
+ if (err)
+ goto error;
+
x->km.state = XFRM_STATE_VALID;
error:
--
1.6.5.2
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH] xfrm: Fix initialize repl field of struct xfrm_state
2011-03-21 5:45 xfrm: Fix initialize repl field of struct xfrm_state Wei Yongjun
@ 2011-03-21 5:48 ` Wei Yongjun
2011-03-21 5:55 ` David Miller
1 sibling, 0 replies; 18+ messages in thread
From: Wei Yongjun @ 2011-03-21 5:48 UTC (permalink / raw)
To: netdev, David Miller, Steffen Klassert
Commit 'xfrm: Move IPsec replay detection functions to a separate file'
(9fdc4883d92d20842c5acea77a4a21bb1574b495)
introduce repl field to struct xfrm_state, and only initialize it
under SA's netlink create path, the other path, such as pf_key, the
repl field remaining uninitialize. So if the SA is created by pf_key,
any input packet with SA's encryption algorithm will cause panic.
int xfrm_input()
{
...
x->repl->advance(x, seq);
...
}
This patch fixed to init default xfrm_replay in xfrm_init_state().
Pid: 0, comm: swapper Not tainted 2.6.38-next+ #14 Bochs Bochs
EIP: 0060:[<c078e5d5>] EFLAGS: 00010206 CPU: 0
EIP is at xfrm_input+0x31c/0x4cc
EAX: dd839c00 EBX: 00000084 ECX: 00000000 EDX: 01000000
ESI: dd839c00 EDI: de3a0780 EBP: dec1de88 ESP: dec1de64
DS: 007b ES: 007b FS: 00d8 GS: 00e0 SS: 0068
Process swapper (pid: 0, ti=dec1c000 task=c09c0f20 task.ti=c0992000)
Stack:
00000000 00000000 00000002 c0ba27c0 00100000 01000000 de3a0798 c0ba27c0
00000033 dec1de98 c0786848 00000000 de3a0780 dec1dea4 c0786868 00000000
dec1debc c074ee56 e1da6b8c de3a0780 c074ed44 de3a07a8 dec1decc c074ef32
Call Trace:
[<c0786848>] xfrm4_rcv_encap+0x22/0x27
[<c0786868>] xfrm4_rcv+0x1b/0x1d
[<c074ee56>] ip_local_deliver_finish+0x112/0x1b1
[<c074ed44>] ? ip_local_deliver_finish+0x0/0x1b1
[<c074ef32>] NF_HOOK.clone.1+0x3d/0x44
[<c074ef77>] ip_local_deliver+0x3e/0x44
[<c074ed44>] ? ip_local_deliver_finish+0x0/0x1b1
[<c074ec03>] ip_rcv_finish+0x30a/0x332
[<c074e8f9>] ? ip_rcv_finish+0x0/0x332
[<c074ef32>] NF_HOOK.clone.1+0x3d/0x44
[<c074f188>] ip_rcv+0x20b/0x247
[<c074e8f9>] ? ip_rcv_finish+0x0/0x332
[<c072797d>] __netif_receive_skb+0x373/0x399
[<c0727bc1>] netif_receive_skb+0x4b/0x51
[<e0817e2a>] cp_rx_poll+0x210/0x2c4 [8139cp]
[<c072818f>] net_rx_action+0x9a/0x17d
[<c0445b5c>] __do_softirq+0xa1/0x149
[<c0445abb>] ? __do_softirq+0x0/0x149
Signed-off-by: Wei Yongjun <yjwei@cn.fujitsu.com>
---
net/xfrm/xfrm_state.c | 4 ++++
1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
index d575f05..4274e11 100644
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c
@@ -1980,6 +1980,10 @@ int xfrm_init_state(struct xfrm_state *x)
if (x->outer_mode == NULL)
goto error;
+ err = xfrm_init_replay(x);
+ if (err)
+ goto error;
+
x->km.state = XFRM_STATE_VALID;
error:
--
1.6.5.2
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: xfrm: Fix initialize repl field of struct xfrm_state
2011-03-21 5:45 xfrm: Fix initialize repl field of struct xfrm_state Wei Yongjun
2011-03-21 5:48 ` [PATCH] " Wei Yongjun
@ 2011-03-21 5:55 ` David Miller
2011-03-21 6:36 ` Wei Yongjun
1 sibling, 1 reply; 18+ messages in thread
From: David Miller @ 2011-03-21 5:55 UTC (permalink / raw)
To: yjwei; +Cc: netdev, steffen.klassert
From: Wei Yongjun <yjwei@cn.fujitsu.com>
Date: Mon, 21 Mar 2011 13:45:39 +0800
> Commit 'xfrm: Move IPsec replay detection functions to a separate file'
> (9fdc4883d92d20842c5acea77a4a21bb1574b495)
> introduce repl field to struct xfrm_state, and only initialize it
> under SA's netlink create path, the other path, such as pf_key, the
> repl field remaining uninitialize. So if the SA is created by pf_key,
> any input packet with SA's encryption algorithm will cause panic.
Please, either add an xfrm_init_replay() call to the appropriate spot
in net/key/af_key.c or, if possible, only have the one call in
xfrm_init_state(). Don't leave two calls, one in xfrm_user.c and one
in xfrm_state.c
Anyways, I don't think just making one call from xfrm_init_state() is
possible, because the replay settings need to be assigned before we
can properly call xfrm_init_replay().
Therefore, please fix this by adding the necessary call to af_key.c
Thanks.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: xfrm: Fix initialize repl field of struct xfrm_state
2011-03-21 5:55 ` David Miller
@ 2011-03-21 6:36 ` Wei Yongjun
2011-03-21 6:46 ` David Miller
0 siblings, 1 reply; 18+ messages in thread
From: Wei Yongjun @ 2011-03-21 6:36 UTC (permalink / raw)
To: David Miller; +Cc: netdev, steffen.klassert
> From: Wei Yongjun <yjwei@cn.fujitsu.com>
> Date: Mon, 21 Mar 2011 13:45:39 +0800
>
>> Commit 'xfrm: Move IPsec replay detection functions to a separate file'
>> (9fdc4883d92d20842c5acea77a4a21bb1574b495)
>> introduce repl field to struct xfrm_state, and only initialize it
>> under SA's netlink create path, the other path, such as pf_key, the
>> repl field remaining uninitialize. So if the SA is created by pf_key,
>> any input packet with SA's encryption algorithm will cause panic.
> Please, either add an xfrm_init_replay() call to the appropriate spot
> in net/key/af_key.c or, if possible, only have the one call in
> xfrm_init_state(). Don't leave two calls, one in xfrm_user.c and one
> in xfrm_state.c
>
> Anyways, I don't think just making one call from xfrm_init_state() is
> possible, because the replay settings need to be assigned before we
> can properly call xfrm_init_replay().
>
> Therefore, please fix this by adding the necessary call to af_key.c
Sorry for not said clearly, at the first time I want to do like this.
But when I grep 'xfrm_init_state', it be used in many place, not
any pf_key, but also XFRM MIGRATE, ipcomp, ipcomp6. So I did this ugly
patch by add this to xfrm_init_state() to avoid dup code.
Not sure whether the other case like ipcomp/ipcomp6 etc can cause panic, if
it panic, maybe we can fix by introduce new xfrm_init_replay() function
like to assign the default reply function.
int xfrm_init_replay(struct xfrm_state *x)
{
x->repl = &xfrm_replay_legacy;
return 0;
}
and change the orig xfrm_init_replay to xfrm_update_replay()?
Or dup those code to all used place?
If I was wrong, I will fix this by adding the necessary call to af_key.c.
Thanks.
> Thanks.
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: xfrm: Fix initialize repl field of struct xfrm_state
2011-03-21 6:36 ` Wei Yongjun
@ 2011-03-21 6:46 ` David Miller
2011-03-21 6:49 ` Wei Yongjun
` (2 more replies)
0 siblings, 3 replies; 18+ messages in thread
From: David Miller @ 2011-03-21 6:46 UTC (permalink / raw)
To: yjwei; +Cc: netdev, steffen.klassert
From: Wei Yongjun <yjwei@cn.fujitsu.com>
Date: Mon, 21 Mar 2011 14:36:45 +0800
> Sorry for not said clearly, at the first time I want to do like this.
> But when I grep 'xfrm_init_state', it be used in many place, not
> any pf_key, but also XFRM MIGRATE, ipcomp, ipcomp6. So I did this ugly
> patch by add this to xfrm_init_state() to avoid dup code.
>
> Not sure whether the other case like ipcomp/ipcomp6 etc can cause panic, if
> it panic, maybe we can fix by introduce new xfrm_init_replay() function
> like to assign the default reply function.
> int xfrm_init_replay(struct xfrm_state *x)
> {
> x->repl = &xfrm_replay_legacy;
> return 0;
> }
> and change the orig xfrm_init_replay to xfrm_update_replay()?
> Or dup those code to all used place?
>
> If I was wrong, I will fix this by adding the necessary call to af_key.c.
Ok, thanks for the explanation.
I think there is a simple way out of this:
1) Rename current xfrm_init_state to __xfrm_init_state, add
"bool init_replay" argument. Add the xfrm_init_replay()
call, as in your patch, but conditionalized on this boolean.
2) Implement xfrm_init_state as inline, which calls
__xfrm_init_state(..., true)
3) Replace xfrm_init_state() call in xfrm_user.c with
__xfrm_init_state(..., false)
This seems to avoid all the problems. We don't need to touch every
caller, and we avoid initializing the replay state twice in xfrm_user
Ok?
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: xfrm: Fix initialize repl field of struct xfrm_state
2011-03-21 6:46 ` David Miller
@ 2011-03-21 6:49 ` Wei Yongjun
2011-03-21 7:54 ` Steffen Klassert
2011-03-21 7:44 ` [PATCH v2] " Wei Yongjun
2011-03-21 8:25 ` Steffen Klassert
2 siblings, 1 reply; 18+ messages in thread
From: Wei Yongjun @ 2011-03-21 6:49 UTC (permalink / raw)
To: David Miller; +Cc: netdev, steffen.klassert
> From: Wei Yongjun <yjwei@cn.fujitsu.com>
> Date: Mon, 21 Mar 2011 14:36:45 +0800
>
>> Sorry for not said clearly, at the first time I want to do like this.
>> But when I grep 'xfrm_init_state', it be used in many place, not
>> any pf_key, but also XFRM MIGRATE, ipcomp, ipcomp6. So I did this ugly
>> patch by add this to xfrm_init_state() to avoid dup code.
>>
>> Not sure whether the other case like ipcomp/ipcomp6 etc can cause panic, if
>> it panic, maybe we can fix by introduce new xfrm_init_replay() function
>> like to assign the default reply function.
>> int xfrm_init_replay(struct xfrm_state *x)
>> {
>> x->repl = &xfrm_replay_legacy;
>> return 0;
>> }
>> and change the orig xfrm_init_replay to xfrm_update_replay()?
>> Or dup those code to all used place?
>>
>> If I was wrong, I will fix this by adding the necessary call to af_key.c.
> Ok, thanks for the explanation.
>
> I think there is a simple way out of this:
>
> 1) Rename current xfrm_init_state to __xfrm_init_state, add
> "bool init_replay" argument. Add the xfrm_init_replay()
> call, as in your patch, but conditionalized on this boolean.
>
> 2) Implement xfrm_init_state as inline, which calls
> __xfrm_init_state(..., true)
>
> 3) Replace xfrm_init_state() call in xfrm_user.c with
> __xfrm_init_state(..., false)
>
> This seems to avoid all the problems. We don't need to touch every
> caller, and we avoid initializing the replay state twice in xfrm_user
>
> Ok?
This OK, I will do this change, thanks.
>
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v2] xfrm: Fix initialize repl field of struct xfrm_state
2011-03-21 6:46 ` David Miller
2011-03-21 6:49 ` Wei Yongjun
@ 2011-03-21 7:44 ` Wei Yongjun
2011-03-21 8:00 ` David Miller
2011-03-21 8:25 ` Steffen Klassert
2 siblings, 1 reply; 18+ messages in thread
From: Wei Yongjun @ 2011-03-21 7:44 UTC (permalink / raw)
To: David Miller; +Cc: netdev, steffen.klassert
Commit 'xfrm: Move IPsec replay detection functions to a separate file'
(9fdc4883d92d20842c5acea77a4a21bb1574b495)
introduce repl field to struct xfrm_state, and only initialize it
under SA's netlink create path, the other path, such as pf_key,
ipcomp/ipcomp6 etc, the repl field remaining uninitialize. So if
the SA is created by pf_key, any input packet with SA's encryption
algorithm will cause panic.
int xfrm_input()
{
...
x->repl->advance(x, seq);
...
}
This patch fixed it by introduce new function __xfrm_init_state().
Pid: 0, comm: swapper Not tainted 2.6.38-next+ #14 Bochs Bochs
EIP: 0060:[<c078e5d5>] EFLAGS: 00010206 CPU: 0
EIP is at xfrm_input+0x31c/0x4cc
EAX: dd839c00 EBX: 00000084 ECX: 00000000 EDX: 01000000
ESI: dd839c00 EDI: de3a0780 EBP: dec1de88 ESP: dec1de64
DS: 007b ES: 007b FS: 00d8 GS: 00e0 SS: 0068
Process swapper (pid: 0, ti=dec1c000 task=c09c0f20 task.ti=c0992000)
Stack:
00000000 00000000 00000002 c0ba27c0 00100000 01000000 de3a0798 c0ba27c0
00000033 dec1de98 c0786848 00000000 de3a0780 dec1dea4 c0786868 00000000
dec1debc c074ee56 e1da6b8c de3a0780 c074ed44 de3a07a8 dec1decc c074ef32
Call Trace:
[<c0786848>] xfrm4_rcv_encap+0x22/0x27
[<c0786868>] xfrm4_rcv+0x1b/0x1d
[<c074ee56>] ip_local_deliver_finish+0x112/0x1b1
[<c074ed44>] ? ip_local_deliver_finish+0x0/0x1b1
[<c074ef32>] NF_HOOK.clone.1+0x3d/0x44
[<c074ef77>] ip_local_deliver+0x3e/0x44
[<c074ed44>] ? ip_local_deliver_finish+0x0/0x1b1
[<c074ec03>] ip_rcv_finish+0x30a/0x332
[<c074e8f9>] ? ip_rcv_finish+0x0/0x332
[<c074ef32>] NF_HOOK.clone.1+0x3d/0x44
[<c074f188>] ip_rcv+0x20b/0x247
[<c074e8f9>] ? ip_rcv_finish+0x0/0x332
[<c072797d>] __netif_receive_skb+0x373/0x399
[<c0727bc1>] netif_receive_skb+0x4b/0x51
[<e0817e2a>] cp_rx_poll+0x210/0x2c4 [8139cp]
[<c072818f>] net_rx_action+0x9a/0x17d
[<c0445b5c>] __do_softirq+0xa1/0x149
[<c0445abb>] ? __do_softirq+0x0/0x149
Signed-off-by: Wei Yongjun <yjwei@cn.fujitsu.com>
---
include/net/xfrm.h | 1 +
net/xfrm/xfrm_state.c | 13 ++++++++++++-
net/xfrm/xfrm_user.c | 2 +-
3 files changed, 14 insertions(+), 2 deletions(-)
diff --git a/include/net/xfrm.h b/include/net/xfrm.h
index 42a8c32..cffa5dc 100644
--- a/include/net/xfrm.h
+++ b/include/net/xfrm.h
@@ -1430,6 +1430,7 @@ extern void xfrm_spd_getinfo(struct net *net, struct xfrmk_spdinfo *si);
extern u32 xfrm_replay_seqhi(struct xfrm_state *x, __be32 net_seq);
extern int xfrm_init_replay(struct xfrm_state *x);
extern int xfrm_state_mtu(struct xfrm_state *x, int mtu);
+extern int __xfrm_init_state(struct xfrm_state *x, bool init_replay);
extern int xfrm_init_state(struct xfrm_state *x);
extern int xfrm_prepare_input(struct xfrm_state *x, struct sk_buff *skb);
extern int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi,
diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
index d575f05..ff96b14 100644
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c
@@ -1907,7 +1907,7 @@ int xfrm_state_mtu(struct xfrm_state *x, int mtu)
return res;
}
-int xfrm_init_state(struct xfrm_state *x)
+int __xfrm_init_state(struct xfrm_state *x, bool init_replay)
{
struct xfrm_state_afinfo *afinfo;
struct xfrm_mode *inner_mode;
@@ -1980,12 +1980,23 @@ int xfrm_init_state(struct xfrm_state *x)
if (x->outer_mode == NULL)
goto error;
+ if (init_replay) {
+ err = xfrm_init_replay(x);
+ if (err)
+ goto error;
+ }
+
x->km.state = XFRM_STATE_VALID;
error:
return err;
}
+int xfrm_init_state(struct xfrm_state *x)
+{
+ return __xfrm_init_state(x, true);
+}
+
EXPORT_SYMBOL(xfrm_init_state);
int __net_init xfrm_state_init(struct net *net)
diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index 706385a..fc152d2 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -511,7 +511,7 @@ static struct xfrm_state *xfrm_state_construct(struct net *net,
xfrm_mark_get(attrs, &x->mark);
- err = xfrm_init_state(x);
+ err = __xfrm_init_state(x, false);
if (err)
goto error;
--
1.6.5.2
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: xfrm: Fix initialize repl field of struct xfrm_state
2011-03-21 6:49 ` Wei Yongjun
@ 2011-03-21 7:54 ` Steffen Klassert
0 siblings, 0 replies; 18+ messages in thread
From: Steffen Klassert @ 2011-03-21 7:54 UTC (permalink / raw)
To: Wei Yongjun; +Cc: David Miller, netdev
Thanks for catching this!
On Mon, Mar 21, 2011 at 02:49:14PM +0800, Wei Yongjun wrote:
> > Ok, thanks for the explanation.
> >
> > I think there is a simple way out of this:
> >
> > 1) Rename current xfrm_init_state to __xfrm_init_state, add
> > "bool init_replay" argument. Add the xfrm_init_replay()
> > call, as in your patch, but conditionalized on this boolean.
> >
> > 2) Implement xfrm_init_state as inline, which calls
> > __xfrm_init_state(..., true)
> >
> > 3) Replace xfrm_init_state() call in xfrm_user.c with
> > __xfrm_init_state(..., false)
> >
> > This seems to avoid all the problems. We don't need to touch every
> > caller, and we avoid initializing the replay state twice in xfrm_user
> >
> > Ok?
>
> This OK, I will do this change, thanks.
>
This looks ok for me too. I just noticed that we also need to clone the
replay_esn/preplay_esn informations and to reinitialize the sequence
number counting on XFRM MIGRATE from the original state before we
call xfrm_init_replay.
I'll fix this up once your patch is applied.
Thanks,
Steffen
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2] xfrm: Fix initialize repl field of struct xfrm_state
2011-03-21 7:44 ` [PATCH v2] " Wei Yongjun
@ 2011-03-21 8:00 ` David Miller
2011-03-21 8:01 ` David Miller
2011-03-21 8:37 ` [PATCH v3] " Wei Yongjun
0 siblings, 2 replies; 18+ messages in thread
From: David Miller @ 2011-03-21 8:00 UTC (permalink / raw)
To: yjwei; +Cc: netdev, steffen.klassert
From: Wei Yongjun <yjwei@cn.fujitsu.com>
Date: Mon, 21 Mar 2011 15:44:03 +0800
> -int xfrm_init_state(struct xfrm_state *x)
> +int __xfrm_init_state(struct xfrm_state *x, bool init_replay)
Since it is referenced by modules, this new function symbol must
be exported to them.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2] xfrm: Fix initialize repl field of struct xfrm_state
2011-03-21 8:00 ` David Miller
@ 2011-03-21 8:01 ` David Miller
2011-03-21 8:37 ` [PATCH v3] " Wei Yongjun
1 sibling, 0 replies; 18+ messages in thread
From: David Miller @ 2011-03-21 8:01 UTC (permalink / raw)
To: yjwei; +Cc: netdev, steffen.klassert
From: David Miller <davem@davemloft.net>
Date: Mon, 21 Mar 2011 01:00:37 -0700 (PDT)
> From: Wei Yongjun <yjwei@cn.fujitsu.com>
> Date: Mon, 21 Mar 2011 15:44:03 +0800
>
>> -int xfrm_init_state(struct xfrm_state *x)
>> +int __xfrm_init_state(struct xfrm_state *x, bool init_replay)
>
> Since it is referenced by modules, this new function symbol must
> be exported to them.
BTW, this is one of the reasons I explicitly mentioned to make
this an inline function. :-)
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: xfrm: Fix initialize repl field of struct xfrm_state
2011-03-21 6:46 ` David Miller
2011-03-21 6:49 ` Wei Yongjun
2011-03-21 7:44 ` [PATCH v2] " Wei Yongjun
@ 2011-03-21 8:25 ` Steffen Klassert
2011-03-21 9:10 ` Wei Yongjun
2 siblings, 1 reply; 18+ messages in thread
From: Steffen Klassert @ 2011-03-21 8:25 UTC (permalink / raw)
To: David Miller; +Cc: yjwei, netdev
On Sun, Mar 20, 2011 at 11:46:06PM -0700, David Miller wrote:
>
> Ok, thanks for the explanation.
>
> I think there is a simple way out of this:
>
> 1) Rename current xfrm_init_state to __xfrm_init_state, add
> "bool init_replay" argument. Add the xfrm_init_replay()
> call, as in your patch, but conditionalized on this boolean.
>
> 2) Implement xfrm_init_state as inline, which calls
> __xfrm_init_state(..., true)
>
> 3) Replace xfrm_init_state() call in xfrm_user.c with
> __xfrm_init_state(..., false)
>
> This seems to avoid all the problems. We don't need to touch every
> caller, and we avoid initializing the replay state twice in xfrm_user
>
Btw, looking a bit closer to this. I think it would look a bit cleaner
if we would add the xfrm_init_replay() call to xfrm_init_state() and
to move the xfrm_init_state() call in xfrm_state_construct() behind
the assign of the replay settings.
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v3] xfrm: Fix initialize repl field of struct xfrm_state
2011-03-21 8:00 ` David Miller
2011-03-21 8:01 ` David Miller
@ 2011-03-21 8:37 ` Wei Yongjun
2011-03-22 1:08 ` David Miller
1 sibling, 1 reply; 18+ messages in thread
From: Wei Yongjun @ 2011-03-21 8:37 UTC (permalink / raw)
To: David Miller; +Cc: netdev, steffen.klassert
Commit 'xfrm: Move IPsec replay detection functions to a separate file'
(9fdc4883d92d20842c5acea77a4a21bb1574b495)
introduce repl field to struct xfrm_state, and only initialize it
under SA's netlink create path, the other path, such as pf_key,
ipcomp/ipcomp6 etc, the repl field remaining uninitialize. So if
the SA is created by pf_key, any input packet with SA's encryption
algorithm will cause panic.
int xfrm_input()
{
...
x->repl->advance(x, seq);
...
}
This patch fixed it by introduce new function __xfrm_init_state().
Pid: 0, comm: swapper Not tainted 2.6.38-next+ #14 Bochs Bochs
EIP: 0060:[<c078e5d5>] EFLAGS: 00010206 CPU: 0
EIP is at xfrm_input+0x31c/0x4cc
EAX: dd839c00 EBX: 00000084 ECX: 00000000 EDX: 01000000
ESI: dd839c00 EDI: de3a0780 EBP: dec1de88 ESP: dec1de64
DS: 007b ES: 007b FS: 00d8 GS: 00e0 SS: 0068
Process swapper (pid: 0, ti=dec1c000 task=c09c0f20 task.ti=c0992000)
Stack:
00000000 00000000 00000002 c0ba27c0 00100000 01000000 de3a0798 c0ba27c0
00000033 dec1de98 c0786848 00000000 de3a0780 dec1dea4 c0786868 00000000
dec1debc c074ee56 e1da6b8c de3a0780 c074ed44 de3a07a8 dec1decc c074ef32
Call Trace:
[<c0786848>] xfrm4_rcv_encap+0x22/0x27
[<c0786868>] xfrm4_rcv+0x1b/0x1d
[<c074ee56>] ip_local_deliver_finish+0x112/0x1b1
[<c074ed44>] ? ip_local_deliver_finish+0x0/0x1b1
[<c074ef32>] NF_HOOK.clone.1+0x3d/0x44
[<c074ef77>] ip_local_deliver+0x3e/0x44
[<c074ed44>] ? ip_local_deliver_finish+0x0/0x1b1
[<c074ec03>] ip_rcv_finish+0x30a/0x332
[<c074e8f9>] ? ip_rcv_finish+0x0/0x332
[<c074ef32>] NF_HOOK.clone.1+0x3d/0x44
[<c074f188>] ip_rcv+0x20b/0x247
[<c074e8f9>] ? ip_rcv_finish+0x0/0x332
[<c072797d>] __netif_receive_skb+0x373/0x399
[<c0727bc1>] netif_receive_skb+0x4b/0x51
[<e0817e2a>] cp_rx_poll+0x210/0x2c4 [8139cp]
[<c072818f>] net_rx_action+0x9a/0x17d
[<c0445b5c>] __do_softirq+0xa1/0x149
[<c0445abb>] ? __do_softirq+0x0/0x149
Signed-off-by: Wei Yongjun <yjwei@cn.fujitsu.com>
---
Changelog:
v2 -> v3 export __xfrm_init_state().
---
include/net/xfrm.h | 1 +
net/xfrm/xfrm_state.c | 15 ++++++++++++++-
net/xfrm/xfrm_user.c | 2 +-
3 files changed, 16 insertions(+), 2 deletions(-)
diff --git a/include/net/xfrm.h b/include/net/xfrm.h
index 42a8c32..cffa5dc 100644
--- a/include/net/xfrm.h
+++ b/include/net/xfrm.h
@@ -1430,6 +1430,7 @@ extern void xfrm_spd_getinfo(struct net *net, struct xfrmk_spdinfo *si);
extern u32 xfrm_replay_seqhi(struct xfrm_state *x, __be32 net_seq);
extern int xfrm_init_replay(struct xfrm_state *x);
extern int xfrm_state_mtu(struct xfrm_state *x, int mtu);
+extern int __xfrm_init_state(struct xfrm_state *x, bool init_replay);
extern int xfrm_init_state(struct xfrm_state *x);
extern int xfrm_prepare_input(struct xfrm_state *x, struct sk_buff *skb);
extern int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi,
diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
index d575f05..f83a3d1 100644
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c
@@ -1907,7 +1907,7 @@ int xfrm_state_mtu(struct xfrm_state *x, int mtu)
return res;
}
-int xfrm_init_state(struct xfrm_state *x)
+int __xfrm_init_state(struct xfrm_state *x, bool init_replay)
{
struct xfrm_state_afinfo *afinfo;
struct xfrm_mode *inner_mode;
@@ -1980,12 +1980,25 @@ int xfrm_init_state(struct xfrm_state *x)
if (x->outer_mode == NULL)
goto error;
+ if (init_replay) {
+ err = xfrm_init_replay(x);
+ if (err)
+ goto error;
+ }
+
x->km.state = XFRM_STATE_VALID;
error:
return err;
}
+EXPORT_SYMBOL(__xfrm_init_state);
+
+int xfrm_init_state(struct xfrm_state *x)
+{
+ return __xfrm_init_state(x, true);
+}
+
EXPORT_SYMBOL(xfrm_init_state);
int __net_init xfrm_state_init(struct net *net)
diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index 706385a..fc152d2 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -511,7 +511,7 @@ static struct xfrm_state *xfrm_state_construct(struct net *net,
xfrm_mark_get(attrs, &x->mark);
- err = xfrm_init_state(x);
+ err = __xfrm_init_state(x, false);
if (err)
goto error;
--
1.6.5.2
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: xfrm: Fix initialize repl field of struct xfrm_state
2011-03-21 8:25 ` Steffen Klassert
@ 2011-03-21 9:10 ` Wei Yongjun
2011-03-21 9:18 ` Wei Yongjun
0 siblings, 1 reply; 18+ messages in thread
From: Wei Yongjun @ 2011-03-21 9:10 UTC (permalink / raw)
To: Steffen Klassert; +Cc: David Miller, netdev
> On Sun, Mar 20, 2011 at 11:46:06PM -0700, David Miller wrote:
>> Ok, thanks for the explanation.
>>
>> I think there is a simple way out of this:
>>
>> 1) Rename current xfrm_init_state to __xfrm_init_state, add
>> "bool init_replay" argument. Add the xfrm_init_replay()
>> call, as in your patch, but conditionalized on this boolean.
>>
>> 2) Implement xfrm_init_state as inline, which calls
>> __xfrm_init_state(..., true)
>>
>> 3) Replace xfrm_init_state() call in xfrm_user.c with
>> __xfrm_init_state(..., false)
>>
>> This seems to avoid all the problems. We don't need to touch every
>> caller, and we avoid initializing the replay state twice in xfrm_user
>>
> Btw, looking a bit closer to this. I think it would look a bit cleaner
> if we would add the xfrm_init_replay() call to xfrm_init_state() and
> to move the xfrm_init_state() call in xfrm_state_construct() behind
> the assign of the replay settings.
The xfrm_init_replay() should be call after the call to
xfrm_update_ae_params(x, attrs);
since xfrm_update_ae_params() may update the replay_esn.
So we need move the xfrm_init_state() call just before return x.
The other issue:
static void xfrm_update_ae_params()
{
...
memcpy(x->replay_esn, replay_esn,
xfrm_replay_state_esn_len(replay_esn));
...
}
the memcpy() may cause memory overlap if we build a special
nl_data, we should free it and then do kmemdup()?
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: xfrm: Fix initialize repl field of struct xfrm_state
2011-03-21 9:10 ` Wei Yongjun
@ 2011-03-21 9:18 ` Wei Yongjun
2011-03-21 12:06 ` Steffen Klassert
0 siblings, 1 reply; 18+ messages in thread
From: Wei Yongjun @ 2011-03-21 9:18 UTC (permalink / raw)
To: Steffen Klassert; +Cc: David Miller, netdev
>> On Sun, Mar 20, 2011 at 11:46:06PM -0700, David Miller wrote:
>>> Ok, thanks for the explanation.
>>>
>>> I think there is a simple way out of this:
>>>
>>> 1) Rename current xfrm_init_state to __xfrm_init_state, add
>>> "bool init_replay" argument. Add the xfrm_init_replay()
>>> call, as in your patch, but conditionalized on this boolean.
>>>
>>> 2) Implement xfrm_init_state as inline, which calls
>>> __xfrm_init_state(..., true)
>>>
>>> 3) Replace xfrm_init_state() call in xfrm_user.c with
>>> __xfrm_init_state(..., false)
>>>
>>> This seems to avoid all the problems. We don't need to touch every
>>> caller, and we avoid initializing the replay state twice in xfrm_user
>>>
>> Btw, looking a bit closer to this. I think it would look a bit cleaner
>> if we would add the xfrm_init_replay() call to xfrm_init_state() and
>> to move the xfrm_init_state() call in xfrm_state_construct() behind
>> the assign of the replay settings.
> The xfrm_init_replay() should be call after the call to
> xfrm_update_ae_params(x, attrs);
> since xfrm_update_ae_params() may update the replay_esn.
>
> So we need move the xfrm_init_state() call just before return x.
Oh, sorry, the memcpy looks like dup code since we used
kmemdup. It is the same attr XFRMA_REPLAY_ESN_VAL.
> The other issue:
> static void xfrm_update_ae_params()
> {
> ...
> memcpy(x->replay_esn, replay_esn,
> xfrm_replay_state_esn_len(replay_esn));
> ...
> }
>
> the memcpy() may cause memory overlap if we build a special
> nl_data, we should free it and then do kmemdup()?
>
>> --
>> To unsubscribe from this list: send the line "unsubscribe netdev" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: xfrm: Fix initialize repl field of struct xfrm_state
2011-03-21 9:18 ` Wei Yongjun
@ 2011-03-21 12:06 ` Steffen Klassert
2011-03-22 1:04 ` Wei Yongjun
0 siblings, 1 reply; 18+ messages in thread
From: Steffen Klassert @ 2011-03-21 12:06 UTC (permalink / raw)
To: Wei Yongjun; +Cc: David Miller, netdev
On Mon, Mar 21, 2011 at 05:18:18PM +0800, Wei Yongjun wrote:
>
> >> Btw, looking a bit closer to this. I think it would look a bit cleaner
> >> if we would add the xfrm_init_replay() call to xfrm_init_state() and
> >> to move the xfrm_init_state() call in xfrm_state_construct() behind
> >> the assign of the replay settings.
> > The xfrm_init_replay() should be call after the call to
> > xfrm_update_ae_params(x, attrs);
> > since xfrm_update_ae_params() may update the replay_esn.
> >
> > So we need move the xfrm_init_state() call just before return x.
>
>
> Oh, sorry, the memcpy looks like dup code since we used
> kmemdup. It is the same attr XFRMA_REPLAY_ESN_VAL.
>
Indeed, we don't need the memcpy here because we do a kmemdup when we
allocate repay_esn/preplay_esn. But we need to memcpy if we call
xfrm_update_ae_params() from xfrm_new_ae().
So we could just replace the kmemdup by kmalloc when we allocate
repay_esn/preplay_esn and move xfrm_init_state() at the end of the
function, as you suggested. xfrm_update_ae_params() would initialize
x->replay_esn and x->preplay_esn properly then.
> > The other issue:
> > static void xfrm_update_ae_params()
> > {
> > ...
> > memcpy(x->replay_esn, replay_esn,
> > xfrm_replay_state_esn_len(replay_esn));
> > ...
> > }
> >
> > the memcpy() may cause memory overlap if we build a special
> > nl_data, we should free it and then do kmemdup()?
> >
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: xfrm: Fix initialize repl field of struct xfrm_state
2011-03-21 12:06 ` Steffen Klassert
@ 2011-03-22 1:04 ` Wei Yongjun
2011-03-22 13:14 ` Steffen Klassert
0 siblings, 1 reply; 18+ messages in thread
From: Wei Yongjun @ 2011-03-22 1:04 UTC (permalink / raw)
To: Steffen Klassert; +Cc: David Miller, netdev
> On Mon, Mar 21, 2011 at 05:18:18PM +0800, Wei Yongjun wrote:
>>>> Btw, looking a bit closer to this. I think it would look a bit cleaner
>>>> if we would add the xfrm_init_replay() call to xfrm_init_state() and
>>>> to move the xfrm_init_state() call in xfrm_state_construct() behind
>>>> the assign of the replay settings.
>>> The xfrm_init_replay() should be call after the call to
>>> xfrm_update_ae_params(x, attrs);
>>> since xfrm_update_ae_params() may update the replay_esn.
>>>
>>> So we need move the xfrm_init_state() call just before return x.
>>
>> Oh, sorry, the memcpy looks like dup code since we used
>> kmemdup. It is the same attr XFRMA_REPLAY_ESN_VAL.
>>
> Indeed, we don't need the memcpy here because we do a kmemdup when we
> allocate repay_esn/preplay_esn. But we need to memcpy if we call
> xfrm_update_ae_params() from xfrm_new_ae().
>
> So we could just replace the kmemdup by kmalloc when we allocate
> repay_esn/preplay_esn and move xfrm_init_state() at the end of the
> function, as you suggested. xfrm_update_ae_params() would initialize
> x->replay_esn and x->preplay_esn properly then.
BTW, looking into more about this, another path, XFRM_MSG_NEWAE,
can overwrite the x->replay_esn with the nla_data length, which
may larger then the size we malloc.
>>> The other issue:
>>> static void xfrm_update_ae_params()
>>> {
>>> ...
>>> memcpy(x->replay_esn, replay_esn,
>>> xfrm_replay_state_esn_len(replay_esn));
>>> ...
>>> }
>>>
>>> the memcpy() may cause memory overlap if we build a special
>>> nl_data, we should free it and then do kmemdup()?
>>>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v3] xfrm: Fix initialize repl field of struct xfrm_state
2011-03-21 8:37 ` [PATCH v3] " Wei Yongjun
@ 2011-03-22 1:08 ` David Miller
0 siblings, 0 replies; 18+ messages in thread
From: David Miller @ 2011-03-22 1:08 UTC (permalink / raw)
To: yjwei; +Cc: netdev, steffen.klassert
From: Wei Yongjun <yjwei@cn.fujitsu.com>
Date: Mon, 21 Mar 2011 16:37:03 +0800
> Commit 'xfrm: Move IPsec replay detection functions to a separate file'
> (9fdc4883d92d20842c5acea77a4a21bb1574b495)
> introduce repl field to struct xfrm_state, and only initialize it
> under SA's netlink create path, the other path, such as pf_key,
> ipcomp/ipcomp6 etc, the repl field remaining uninitialize. So if
> the SA is created by pf_key, any input packet with SA's encryption
> algorithm will cause panic.
I'll apply this, thanks.
I know you guys are discussing alternative ways to handle this,
but getting the crash fixed in the simplest way now doesn't hurt.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: xfrm: Fix initialize repl field of struct xfrm_state
2011-03-22 1:04 ` Wei Yongjun
@ 2011-03-22 13:14 ` Steffen Klassert
0 siblings, 0 replies; 18+ messages in thread
From: Steffen Klassert @ 2011-03-22 13:14 UTC (permalink / raw)
To: Wei Yongjun; +Cc: David Miller, netdev
On Tue, Mar 22, 2011 at 09:04:38AM +0800, Wei Yongjun wrote:
>
> BTW, looking into more about this, another path, XFRM_MSG_NEWAE,
> can overwrite the x->replay_esn with the nla_data length, which
> may larger then the size we malloc.
>
Yes, I've noticed that yesterday too. I'll fix it up.
Thanks!
^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2011-03-22 13:14 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-03-21 5:45 xfrm: Fix initialize repl field of struct xfrm_state Wei Yongjun
2011-03-21 5:48 ` [PATCH] " Wei Yongjun
2011-03-21 5:55 ` David Miller
2011-03-21 6:36 ` Wei Yongjun
2011-03-21 6:46 ` David Miller
2011-03-21 6:49 ` Wei Yongjun
2011-03-21 7:54 ` Steffen Klassert
2011-03-21 7:44 ` [PATCH v2] " Wei Yongjun
2011-03-21 8:00 ` David Miller
2011-03-21 8:01 ` David Miller
2011-03-21 8:37 ` [PATCH v3] " Wei Yongjun
2011-03-22 1:08 ` David Miller
2011-03-21 8:25 ` Steffen Klassert
2011-03-21 9:10 ` Wei Yongjun
2011-03-21 9:18 ` Wei Yongjun
2011-03-21 12:06 ` Steffen Klassert
2011-03-22 1:04 ` Wei Yongjun
2011-03-22 13:14 ` Steffen Klassert
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).