* Re: Race 1 in net/xfrm/xfrm_algo.c [not found] <CAEHB24-9hXY+TgQKxJB4bE9a9dFD9C+Lan+ShBwpvwaHVAGMFg@mail.gmail.com> @ 2022-07-22 3:16 ` Herbert Xu [not found] ` <CAEHB249ygptvp9wpynMF7zZ2Kcet0+bwLVuVg5UReZHOU1+8HA@mail.gmail.com> 0 siblings, 1 reply; 5+ messages in thread From: Herbert Xu @ 2022-07-22 3:16 UTC (permalink / raw) To: Abhishek Shah Cc: davem, edumazet, kuba, netdev, pabeni, steffen.klassert, linux-kernel, Gabriel Ryan On Thu, Jul 21, 2022 at 12:03:04PM -0400, Abhishek Shah wrote: > Dear Kernel Maintainers, > > We found a race in net/xfrm/xfrm_algo.c. The function *xfrm_probe_algs* updates > the availability field of items in a authentication algorithms list ( > *aalg_list* variable), but this update can occur simultaneously with > another invocation of *xfrm_probe_algs*, leading to double writes and > read/write consistency issues in scenarios where the *status* variable may > vary across the concurrent invocations of the function. This behavior also > occurs with another list with encryption algorithms (*ealg_list* variable) > as well as with the *xfrm_find_algo* function. We thought this is > undesirable given cryptographic logic errors often have security > implications. > > We provide more details below including the trace and reproducing > test cases. What inconsistency are you talking about? An algorithm can always disappear even if it was available earlier. Please state clearly why this is actually a problem rather than relying on some automated test whose results are useless without human interpretation. Thanks, -- Email: Herbert Xu <herbert@gondor.apana.org.au> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt ^ permalink raw reply [flat|nested] 5+ messages in thread
[parent not found: <CAEHB249ygptvp9wpynMF7zZ2Kcet0+bwLVuVg5UReZHOU1+8HA@mail.gmail.com>]
* Re: Race 1 in net/xfrm/xfrm_algo.c [not found] ` <CAEHB249ygptvp9wpynMF7zZ2Kcet0+bwLVuVg5UReZHOU1+8HA@mail.gmail.com> @ 2022-07-29 2:30 ` Herbert Xu 2022-08-04 10:03 ` [PATCH] af_key: Do not call xfrm_probe_algs in parallel Herbert Xu 0 siblings, 1 reply; 5+ messages in thread From: Herbert Xu @ 2022-07-29 2:30 UTC (permalink / raw) To: Abhishek Shah Cc: davem, edumazet, kuba, netdev, pabeni, steffen.klassert, linux-kernel, Gabriel Ryan, Fan Du On Thu, Jul 28, 2022 at 08:00:00PM -0400, Abhishek Shah wrote: > Dear Herbert, > > Thanks for your quick reply and sorry for not being more clear about the > inconsistencies. We identified security implications when algorithms > disappear or reappear during the execution of *compose_sadb_supported*. > > In more detail, > > 1) If after *xfrm_count_pfkey_auth_supported* has finished counting the > available algos, a secondary thread changes the availability of an algo > through *xfrm_probe_algs*, it will return an incorrect number of available > algorithms. If an algo was added during the racing access, the code > allocates a buffer that is smaller than the number of available algorithms > at net/key/af_key.c#L1619 > <https://elixir.bootlin.com/linux/v5.18-rc5/source/net/key/af_key.c#L1619>. > This will result in an out of bounds write when the buffer is later > populated at net/key/af_key.c#L1657 > <https://elixir.bootlin.com/linux/v5.18-rc5/source/net/key/af_key.c#L1657>. OK this is a real bug caused by this commit: commit 283bc9f35bbbcb0e9ab4e6d2427da7f9f710d52d Author: Fan Du <fan.du@windriver.com> Date: Thu Nov 7 17:47:50 2013 +0800 xfrm: Namespacify xfrm state/policy locks It neglected to convert xfrm_probe_algs to namespaces so the previous assumption of exclusive ownership of xfrm_algo_list by the current afkey request is no longer true. Thanks, -- Email: Herbert Xu <herbert@gondor.apana.org.au> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH] af_key: Do not call xfrm_probe_algs in parallel 2022-07-29 2:30 ` Herbert Xu @ 2022-08-04 10:03 ` Herbert Xu 2022-08-09 5:47 ` Steffen Klassert 2022-08-22 16:19 ` Gabriel Ryan 0 siblings, 2 replies; 5+ messages in thread From: Herbert Xu @ 2022-08-04 10:03 UTC (permalink / raw) To: Abhishek Shah Cc: davem, edumazet, kuba, netdev, pabeni, steffen.klassert, linux-kernel, Gabriel Ryan, Fan Du, Steffen Klassert When namespace support was added to xfrm/afkey, it caused the previously single-threaded call to xfrm_probe_algs to become multi-threaded. This is buggy and needs to be fixed with a mutex. Reported-by: Abhishek Shah <abhishek.shah@columbia.edu> Fixes: 283bc9f35bbb ("xfrm: Namespacify xfrm state/policy locks") Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au> diff --git a/net/key/af_key.c b/net/key/af_key.c index fb16d7c4e1b8..20e73643b9c8 100644 --- a/net/key/af_key.c +++ b/net/key/af_key.c @@ -1697,9 +1697,12 @@ static int pfkey_register(struct sock *sk, struct sk_buff *skb, const struct sad pfk->registered |= (1<<hdr->sadb_msg_satype); } + mutex_lock(&pfkey_mutex); xfrm_probe_algs(); supp_skb = compose_sadb_supported(hdr, GFP_KERNEL | __GFP_ZERO); + mutex_unlock(&pfkey_mutex); + if (!supp_skb) { if (hdr->sadb_msg_satype != SADB_SATYPE_UNSPEC) pfk->registered &= ~(1<<hdr->sadb_msg_satype); -- Email: Herbert Xu <herbert@gondor.apana.org.au> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] af_key: Do not call xfrm_probe_algs in parallel 2022-08-04 10:03 ` [PATCH] af_key: Do not call xfrm_probe_algs in parallel Herbert Xu @ 2022-08-09 5:47 ` Steffen Klassert 2022-08-22 16:19 ` Gabriel Ryan 1 sibling, 0 replies; 5+ messages in thread From: Steffen Klassert @ 2022-08-09 5:47 UTC (permalink / raw) To: Herbert Xu Cc: Abhishek Shah, davem, edumazet, kuba, netdev, pabeni, linux-kernel, Gabriel Ryan, Fan Du, Steffen Klassert On Thu, Aug 04, 2022 at 06:03:46PM +0800, Herbert Xu wrote: > When namespace support was added to xfrm/afkey, it caused the > previously single-threaded call to xfrm_probe_algs to become > multi-threaded. This is buggy and needs to be fixed with a mutex. > > Reported-by: Abhishek Shah <abhishek.shah@columbia.edu> > Fixes: 283bc9f35bbb ("xfrm: Namespacify xfrm state/policy locks") > Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au> Applied, thanks a lot Herbert! ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] af_key: Do not call xfrm_probe_algs in parallel 2022-08-04 10:03 ` [PATCH] af_key: Do not call xfrm_probe_algs in parallel Herbert Xu 2022-08-09 5:47 ` Steffen Klassert @ 2022-08-22 16:19 ` Gabriel Ryan 1 sibling, 0 replies; 5+ messages in thread From: Gabriel Ryan @ 2022-08-22 16:19 UTC (permalink / raw) To: Herbert Xu Cc: Abhishek Shah, davem, edumazet, kuba, netdev, pabeni, steffen.klassert, linux-kernel, Fan Du, Steffen Klassert We can confirm we tested this patch and it prevents the race we detected in xfrm_ealg_get_byname / xfrm_probe_algs. Best, Gabe On Thu, Aug 4, 2022 at 6:03 AM Herbert Xu <herbert@gondor.apana.org.au> wrote: > > When namespace support was added to xfrm/afkey, it caused the > previously single-threaded call to xfrm_probe_algs to become > multi-threaded. This is buggy and needs to be fixed with a mutex. > > Reported-by: Abhishek Shah <abhishek.shah@columbia.edu> > Fixes: 283bc9f35bbb ("xfrm: Namespacify xfrm state/policy locks") > Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au> > > diff --git a/net/key/af_key.c b/net/key/af_key.c > index fb16d7c4e1b8..20e73643b9c8 100644 > --- a/net/key/af_key.c > +++ b/net/key/af_key.c > @@ -1697,9 +1697,12 @@ static int pfkey_register(struct sock *sk, struct sk_buff *skb, const struct sad > pfk->registered |= (1<<hdr->sadb_msg_satype); > } > > + mutex_lock(&pfkey_mutex); > xfrm_probe_algs(); > > supp_skb = compose_sadb_supported(hdr, GFP_KERNEL | __GFP_ZERO); > + mutex_unlock(&pfkey_mutex); > + > if (!supp_skb) { > if (hdr->sadb_msg_satype != SADB_SATYPE_UNSPEC) > pfk->registered &= ~(1<<hdr->sadb_msg_satype); > -- > Email: Herbert Xu <herbert@gondor.apana.org.au> > Home Page: http://gondor.apana.org.au/~herbert/ > PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt -- Gabriel Ryan PhD Candidate at Columbia University cs.columbia.edu/~gabe ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-08-22 16:20 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <CAEHB24-9hXY+TgQKxJB4bE9a9dFD9C+Lan+ShBwpvwaHVAGMFg@mail.gmail.com> 2022-07-22 3:16 ` Race 1 in net/xfrm/xfrm_algo.c Herbert Xu [not found] ` <CAEHB249ygptvp9wpynMF7zZ2Kcet0+bwLVuVg5UReZHOU1+8HA@mail.gmail.com> 2022-07-29 2:30 ` Herbert Xu 2022-08-04 10:03 ` [PATCH] af_key: Do not call xfrm_probe_algs in parallel Herbert Xu 2022-08-09 5:47 ` Steffen Klassert 2022-08-22 16:19 ` Gabriel Ryan
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.