Message ID | 4D86E603.8080704@cn.fujitsu.com |
---|---|
State | Changes Requested, archived |
Delegated to: | David Miller |
Headers | show |
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. -- 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
> 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. > -- 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
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? -- 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
> 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. > > -- 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
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 -- 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
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. -- 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
> 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 > -- 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
>> 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 > -- 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
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()? > > -- 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
> 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()? >>> -- 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
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! -- 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
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:
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(-)