* Re: [PATCH V2] Fix a sleep-in-atomic bug in shash_setkey_unaligned
@ 2017-10-03 5:26 ` Herbert Xu
0 siblings, 0 replies; 29+ messages in thread
From: Herbert Xu @ 2017-10-03 5:26 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Jia-Ju Bai, David S. Miller, Neil Horman,
vyasevich-Re5JQEeQqe8AvxtiuMwx3w, Kalle Valo,
Linux Crypto Mailing List, Network Development,
linux-sctp-u79uwXL29TY76Z2rM5mHXA, Linux Wireless List
On Mon, Oct 02, 2017 at 09:18:24PM -0700, Andy Lutomirski wrote:
> > On Oct 2, 2017, at 7:25 PM, Jia-Ju Bai <baijiaju1990@163.com> wrote:
> >
> > The SCTP program may sleep under a spinlock, and the function call path is:
> > sctp_generate_t3_rtx_event (acquire the spinlock)
> > sctp_do_sm
> > sctp_side_effects
> > sctp_cmd_interpreter
> > sctp_make_init_ack
> > sctp_pack_cookie
> > crypto_shash_setkey
> > shash_setkey_unaligned
> > kmalloc(GFP_KERNEL)
>
> I'm going to go out on a limb here: why on Earth is out crypto API so
> full of indirection that we allocate memory at all here?
The crypto API operates on a one key per-tfm basis. So normally
tfm allocation and key setting is done once only and not done on
the data path.
I have looked at the SCTP code and it appears to fit this paradigm.
That is, we should be able to allocate the tfm and set the key when
the key is actually generated via get_random_bytes, rather than every
time the key is used which is not only a waste but as you see runs
into API issues.
Usually if you're invoking setkey from a non-sleeping code-path
you're probably doing something wrong.
As someone else noted recently, there is no single forum for
reviewing code that uses the crypto API so buggy code like this
is not surprising.
> We're synchronously computing a hash of a small amount of data using
> either HMAC-SHA1 or HMAC-SHA256 (determined at runtime) if I read it
> right. There's a sane way to do this that doesn't need kmalloc,
> alloca, or fancy indirection. And then there's crypto_shash_xyz().
There are some legitimate cases where you want to use a different
key for every hashing operation. But so far these are uses have
been very few so there has been no need to provide an API for them.
Cheers,
--
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] 29+ messages in thread
* Re: [PATCH V2] Fix a sleep-in-atomic bug in shash_setkey_unaligned
@ 2017-10-03 5:26 ` Herbert Xu
0 siblings, 0 replies; 29+ messages in thread
From: Herbert Xu @ 2017-10-03 5:26 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Jia-Ju Bai, David S. Miller, Neil Horman,
vyasevich-Re5JQEeQqe8AvxtiuMwx3w, Kalle Valo,
Linux Crypto Mailing List, Network Development,
linux-sctp-u79uwXL29TY76Z2rM5mHXA, Linux Wireless List
On Mon, Oct 02, 2017 at 09:18:24PM -0700, Andy Lutomirski wrote:
> > On Oct 2, 2017, at 7:25 PM, Jia-Ju Bai <baijiaju1990-9Onoh4P/yGk@public.gmane.org> wrote:
> >
> > The SCTP program may sleep under a spinlock, and the function call path is:
> > sctp_generate_t3_rtx_event (acquire the spinlock)
> > sctp_do_sm
> > sctp_side_effects
> > sctp_cmd_interpreter
> > sctp_make_init_ack
> > sctp_pack_cookie
> > crypto_shash_setkey
> > shash_setkey_unaligned
> > kmalloc(GFP_KERNEL)
>
> I'm going to go out on a limb here: why on Earth is out crypto API so
> full of indirection that we allocate memory at all here?
The crypto API operates on a one key per-tfm basis. So normally
tfm allocation and key setting is done once only and not done on
the data path.
I have looked at the SCTP code and it appears to fit this paradigm.
That is, we should be able to allocate the tfm and set the key when
the key is actually generated via get_random_bytes, rather than every
time the key is used which is not only a waste but as you see runs
into API issues.
Usually if you're invoking setkey from a non-sleeping code-path
you're probably doing something wrong.
As someone else noted recently, there is no single forum for
reviewing code that uses the crypto API so buggy code like this
is not surprising.
> We're synchronously computing a hash of a small amount of data using
> either HMAC-SHA1 or HMAC-SHA256 (determined at runtime) if I read it
> right. There's a sane way to do this that doesn't need kmalloc,
> alloca, or fancy indirection. And then there's crypto_shash_xyz().
There are some legitimate cases where you want to use a different
key for every hashing operation. But so far these are uses have
been very few so there has been no need to provide an API for them.
Cheers,
--
Email: Herbert Xu <herbert-lOAM2aK0SrRLBo1qDEOMRrpzq4S04n8Q@public.gmane.org>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH V2] Fix a sleep-in-atomic bug in shash_setkey_unaligned
2017-10-03 5:26 ` Herbert Xu
@ 2017-10-03 16:46 ` Andy Lutomirski
-1 siblings, 0 replies; 29+ messages in thread
From: Andy Lutomirski @ 2017-10-03 16:46 UTC (permalink / raw)
To: Herbert Xu
Cc: Andy Lutomirski, Jia-Ju Bai, David S. Miller, Neil Horman,
vyasevich, Kalle Valo, Linux Crypto Mailing List,
Network Development, linux-sctp, Linux Wireless List
On Mon, Oct 2, 2017 at 10:26 PM, Herbert Xu <herbert@gondor.apana.org.au> wrote:
> On Mon, Oct 02, 2017 at 09:18:24PM -0700, Andy Lutomirski wrote:
>> > On Oct 2, 2017, at 7:25 PM, Jia-Ju Bai <baijiaju1990@163.com> wrote:
>> >
>> > The SCTP program may sleep under a spinlock, and the function call path is:
>> > sctp_generate_t3_rtx_event (acquire the spinlock)
>> > sctp_do_sm
>> > sctp_side_effects
>> > sctp_cmd_interpreter
>> > sctp_make_init_ack
>> > sctp_pack_cookie
>> > crypto_shash_setkey
>> > shash_setkey_unaligned
>> > kmalloc(GFP_KERNEL)
>>
>> I'm going to go out on a limb here: why on Earth is out crypto API so
>> full of indirection that we allocate memory at all here?
>
> The crypto API operates on a one key per-tfm basis. So normally
> tfm allocation and key setting is done once only and not done on
> the data path.
>
> I have looked at the SCTP code and it appears to fit this paradigm.
> That is, we should be able to allocate the tfm and set the key when
> the key is actually generated via get_random_bytes, rather than every
> time the key is used which is not only a waste but as you see runs
> into API issues.
It's a waste because it loses a pre-computation advantage.
The fact that it has memory allocation issues is crypto API's fault,
full stop. There is no legit reason to need to allocate anything.
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH V2] Fix a sleep-in-atomic bug in shash_setkey_unaligned
@ 2017-10-03 16:46 ` Andy Lutomirski
0 siblings, 0 replies; 29+ messages in thread
From: Andy Lutomirski @ 2017-10-03 16:46 UTC (permalink / raw)
To: Herbert Xu
Cc: Andy Lutomirski, Jia-Ju Bai, David S. Miller, Neil Horman,
vyasevich, Kalle Valo, Linux Crypto Mailing List,
Network Development, linux-sctp, Linux Wireless List
On Mon, Oct 2, 2017 at 10:26 PM, Herbert Xu <herbert@gondor.apana.org.au> wrote:
> On Mon, Oct 02, 2017 at 09:18:24PM -0700, Andy Lutomirski wrote:
>> > On Oct 2, 2017, at 7:25 PM, Jia-Ju Bai <baijiaju1990@163.com> wrote:
>> >
>> > The SCTP program may sleep under a spinlock, and the function call path is:
>> > sctp_generate_t3_rtx_event (acquire the spinlock)
>> > sctp_do_sm
>> > sctp_side_effects
>> > sctp_cmd_interpreter
>> > sctp_make_init_ack
>> > sctp_pack_cookie
>> > crypto_shash_setkey
>> > shash_setkey_unaligned
>> > kmalloc(GFP_KERNEL)
>>
>> I'm going to go out on a limb here: why on Earth is out crypto API so
>> full of indirection that we allocate memory at all here?
>
> The crypto API operates on a one key per-tfm basis. So normally
> tfm allocation and key setting is done once only and not done on
> the data path.
>
> I have looked at the SCTP code and it appears to fit this paradigm.
> That is, we should be able to allocate the tfm and set the key when
> the key is actually generated via get_random_bytes, rather than every
> time the key is used which is not only a waste but as you see runs
> into API issues.
It's a waste because it loses a pre-computation advantage.
The fact that it has memory allocation issues is crypto API's fault,
full stop. There is no legit reason to need to allocate anything.
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH V2] Fix a sleep-in-atomic bug in shash_setkey_unaligned
2017-10-03 5:26 ` Herbert Xu
@ 2017-10-03 22:45 ` Marcelo Ricardo Leitner
-1 siblings, 0 replies; 29+ messages in thread
From: Marcelo Ricardo Leitner @ 2017-10-03 22:45 UTC (permalink / raw)
To: Herbert Xu
Cc: Andy Lutomirski, Jia-Ju Bai, David S. Miller, Neil Horman,
vyasevich, Kalle Valo, Linux Crypto Mailing List,
Network Development, linux-sctp, Linux Wireless List
On Tue, Oct 03, 2017 at 01:26:43PM +0800, Herbert Xu wrote:
> On Mon, Oct 02, 2017 at 09:18:24PM -0700, Andy Lutomirski wrote:
> > > On Oct 2, 2017, at 7:25 PM, Jia-Ju Bai <baijiaju1990@163.com> wrote:
> > >
> > > The SCTP program may sleep under a spinlock, and the function call path is:
> > > sctp_generate_t3_rtx_event (acquire the spinlock)
> > > sctp_do_sm
> > > sctp_side_effects
> > > sctp_cmd_interpreter
> > > sctp_make_init_ack
> > > sctp_pack_cookie
> > > crypto_shash_setkey
> > > shash_setkey_unaligned
> > > kmalloc(GFP_KERNEL)
> >
> > I'm going to go out on a limb here: why on Earth is out crypto API so
> > full of indirection that we allocate memory at all here?
>
> The crypto API operates on a one key per-tfm basis. So normally
> tfm allocation and key setting is done once only and not done on
> the data path.
>
> I have looked at the SCTP code and it appears to fit this paradigm.
> That is, we should be able to allocate the tfm and set the key when
> the key is actually generated via get_random_bytes, rather than every
> time the key is used which is not only a waste but as you see runs
> into API issues.
Fair point, but
>
> Usually if you're invoking setkey from a non-sleeping code-path
> you're probably doing something wrong.
Usually but not always. There are 3 calls to that function on SCTP
code:
- pack a cookie, which is sent on an INIT_ACK packet to the client
- unpack the cookie above, after it is sent back by the client on a
COOKIE_ECHO packet
- send a chunk authenticated by a hash
the first two happen during softirq processing, while processing a
packet that was received.
As I explained on the other email, SCTP code is not supposed to store
any information about the peer between the 1st and the 2nd moments
above, to be less vulnerable to DoS attacks (it's planned so by the
RFC), thus why it uses the cookie.
The 3rd one we probably can improve, but I don't think we can do much
about the 2 first ones from the SCTP side.
Note on sctp_sf_do_5_1B_init() how sctp_make_init_ack() is explicitly
called with GFP_ATOMIC, and also on sctp_sf_do_unexpected_init().
Though we can't propagate that to crypto_shash_setkey.
Ideas?
Thanks,
Marcelo
>
> As someone else noted recently, there is no single forum for
> reviewing code that uses the crypto API so buggy code like this
> is not surprising.
>
> > We're synchronously computing a hash of a small amount of data using
> > either HMAC-SHA1 or HMAC-SHA256 (determined at runtime) if I read it
> > right. There's a sane way to do this that doesn't need kmalloc,
> > alloca, or fancy indirection. And then there's crypto_shash_xyz().
>
> There are some legitimate cases where you want to use a different
> key for every hashing operation. But so far these are uses have
> been very few so there has been no need to provide an API for them.
>
> Cheers,
> --
> 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
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sctp" 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] 29+ messages in thread
* Re: [PATCH V2] Fix a sleep-in-atomic bug in shash_setkey_unaligned
@ 2017-10-03 22:45 ` Marcelo Ricardo Leitner
0 siblings, 0 replies; 29+ messages in thread
From: Marcelo Ricardo Leitner @ 2017-10-03 22:45 UTC (permalink / raw)
To: Herbert Xu
Cc: Andy Lutomirski, Jia-Ju Bai, David S. Miller, Neil Horman,
vyasevich, Kalle Valo, Linux Crypto Mailing List,
Network Development, linux-sctp, Linux Wireless List
On Tue, Oct 03, 2017 at 01:26:43PM +0800, Herbert Xu wrote:
> On Mon, Oct 02, 2017 at 09:18:24PM -0700, Andy Lutomirski wrote:
> > > On Oct 2, 2017, at 7:25 PM, Jia-Ju Bai <baijiaju1990@163.com> wrote:
> > >
> > > The SCTP program may sleep under a spinlock, and the function call path is:
> > > sctp_generate_t3_rtx_event (acquire the spinlock)
> > > sctp_do_sm
> > > sctp_side_effects
> > > sctp_cmd_interpreter
> > > sctp_make_init_ack
> > > sctp_pack_cookie
> > > crypto_shash_setkey
> > > shash_setkey_unaligned
> > > kmalloc(GFP_KERNEL)
> >
> > I'm going to go out on a limb here: why on Earth is out crypto API so
> > full of indirection that we allocate memory at all here?
>
> The crypto API operates on a one key per-tfm basis. So normally
> tfm allocation and key setting is done once only and not done on
> the data path.
>
> I have looked at the SCTP code and it appears to fit this paradigm.
> That is, we should be able to allocate the tfm and set the key when
> the key is actually generated via get_random_bytes, rather than every
> time the key is used which is not only a waste but as you see runs
> into API issues.
Fair point, but
>
> Usually if you're invoking setkey from a non-sleeping code-path
> you're probably doing something wrong.
Usually but not always. There are 3 calls to that function on SCTP
code:
- pack a cookie, which is sent on an INIT_ACK packet to the client
- unpack the cookie above, after it is sent back by the client on a
COOKIE_ECHO packet
- send a chunk authenticated by a hash
the first two happen during softirq processing, while processing a
packet that was received.
As I explained on the other email, SCTP code is not supposed to store
any information about the peer between the 1st and the 2nd moments
above, to be less vulnerable to DoS attacks (it's planned so by the
RFC), thus why it uses the cookie.
The 3rd one we probably can improve, but I don't think we can do much
about the 2 first ones from the SCTP side.
Note on sctp_sf_do_5_1B_init() how sctp_make_init_ack() is explicitly
called with GFP_ATOMIC, and also on sctp_sf_do_unexpected_init().
Though we can't propagate that to crypto_shash_setkey.
Ideas?
Thanks,
Marcelo
>
> As someone else noted recently, there is no single forum for
> reviewing code that uses the crypto API so buggy code like this
> is not surprising.
>
> > We're synchronously computing a hash of a small amount of data using
> > either HMAC-SHA1 or HMAC-SHA256 (determined at runtime) if I read it
> > right. There's a sane way to do this that doesn't need kmalloc,
> > alloca, or fancy indirection. And then there's crypto_shash_xyz().
>
> There are some legitimate cases where you want to use a different
> key for every hashing operation. But so far these are uses have
> been very few so there has been no need to provide an API for them.
>
> Cheers,
> --
> 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
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sctp" 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] 29+ messages in thread
* Re: [PATCH V2] Fix a sleep-in-atomic bug in shash_setkey_unaligned
2017-10-03 22:45 ` Marcelo Ricardo Leitner
@ 2017-10-05 3:40 ` Herbert Xu
-1 siblings, 0 replies; 29+ messages in thread
From: Herbert Xu @ 2017-10-05 3:40 UTC (permalink / raw)
To: Marcelo Ricardo Leitner
Cc: Andy Lutomirski, Jia-Ju Bai, David S. Miller, Neil Horman,
vyasevich, Kalle Valo, Linux Crypto Mailing List,
Network Development, linux-sctp, Linux Wireless List
On Tue, Oct 03, 2017 at 07:45:06PM -0300, Marcelo Ricardo Leitner wrote:
>
> > Usually if you're invoking setkey from a non-sleeping code-path
> > you're probably doing something wrong.
>
> Usually but not always. There are 3 calls to that function on SCTP
> code:
> - pack a cookie, which is sent on an INIT_ACK packet to the client
> - unpack the cookie above, after it is sent back by the client on a
> COOKIE_ECHO packet
> - send a chunk authenticated by a hash
I'm not talking about the code-path in question. I'm talking
about the function which generates the secret key in the first
place. AFAICS that's only called in GFP_KERNEL context. What
am I missing?
Cheers,
--
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] 29+ messages in thread
* Re: [PATCH V2] Fix a sleep-in-atomic bug in shash_setkey_unaligned
@ 2017-10-05 3:40 ` Herbert Xu
0 siblings, 0 replies; 29+ messages in thread
From: Herbert Xu @ 2017-10-05 3:40 UTC (permalink / raw)
To: Marcelo Ricardo Leitner
Cc: Andy Lutomirski, Jia-Ju Bai, David S. Miller, Neil Horman,
vyasevich, Kalle Valo, Linux Crypto Mailing List,
Network Development, linux-sctp, Linux Wireless List
On Tue, Oct 03, 2017 at 07:45:06PM -0300, Marcelo Ricardo Leitner wrote:
>
> > Usually if you're invoking setkey from a non-sleeping code-path
> > you're probably doing something wrong.
>
> Usually but not always. There are 3 calls to that function on SCTP
> code:
> - pack a cookie, which is sent on an INIT_ACK packet to the client
> - unpack the cookie above, after it is sent back by the client on a
> COOKIE_ECHO packet
> - send a chunk authenticated by a hash
I'm not talking about the code-path in question. I'm talking
about the function which generates the secret key in the first
place. AFAICS that's only called in GFP_KERNEL context. What
am I missing?
Cheers,
--
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] 29+ messages in thread
* Re: [PATCH V2] Fix a sleep-in-atomic bug in shash_setkey_unaligned
@ 2017-10-05 4:37 ` David Miller
0 siblings, 0 replies; 29+ messages in thread
From: David Miller @ 2017-10-05 4:37 UTC (permalink / raw)
To: herbert
Cc: marcelo.leitner, luto, baijiaju1990, nhorman, vyasevich, kvalo,
linux-crypto, netdev, linux-sctp, linux-wireless
From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Thu, 5 Oct 2017 11:40:54 +0800
> On Tue, Oct 03, 2017 at 07:45:06PM -0300, Marcelo Ricardo Leitner wrote:
>>
>> > Usually if you're invoking setkey from a non-sleeping code-path
>> > you're probably doing something wrong.
>>
>> Usually but not always. There are 3 calls to that function on SCTP
>> code:
>> - pack a cookie, which is sent on an INIT_ACK packet to the client
>> - unpack the cookie above, after it is sent back by the client on a
>> COOKIE_ECHO packet
>> - send a chunk authenticated by a hash
>
> I'm not talking about the code-path in question. I'm talking
> about the function which generates the secret key in the first
> place. AFAICS that's only called in GFP_KERNEL context. What
> am I missing?
The setkey happens in functions like sctp_pack_cookie() and
sctp_unpack_cookie(), which seems to run from software interrupts.
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH V2] Fix a sleep-in-atomic bug in shash_setkey_unaligned
@ 2017-10-05 4:37 ` David Miller
0 siblings, 0 replies; 29+ messages in thread
From: David Miller @ 2017-10-05 4:37 UTC (permalink / raw)
To: herbert-lOAM2aK0SrRLBo1qDEOMRrpzq4S04n8Q
Cc: marcelo.leitner-Re5JQEeQqe8AvxtiuMwx3w,
luto-DgEjT+Ai2ygdnm+yROfE0A, baijiaju1990-9Onoh4P/yGk,
nhorman-2XuSBdqkA4R54TAoqtyWWQ, vyasevich-Re5JQEeQqe8AvxtiuMwx3w,
kvalo-sgV2jX0FEOL9JmXXK+q4OQ,
linux-crypto-u79uwXL29TY76Z2rM5mHXA,
netdev-u79uwXL29TY76Z2rM5mHXA, linux-sctp-u79uwXL29TY76Z2rM5mHXA,
linux-wireless-u79uwXL29TY76Z2rM5mHXA
From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Thu, 5 Oct 2017 11:40:54 +0800
> On Tue, Oct 03, 2017 at 07:45:06PM -0300, Marcelo Ricardo Leitner wrote:
>>
>> > Usually if you're invoking setkey from a non-sleeping code-path
>> > you're probably doing something wrong.
>>
>> Usually but not always. There are 3 calls to that function on SCTP
>> code:
>> - pack a cookie, which is sent on an INIT_ACK packet to the client
>> - unpack the cookie above, after it is sent back by the client on a
>> COOKIE_ECHO packet
>> - send a chunk authenticated by a hash
>
> I'm not talking about the code-path in question. I'm talking
> about the function which generates the secret key in the first
> place. AFAICS that's only called in GFP_KERNEL context. What
> am I missing?
The setkey happens in functions like sctp_pack_cookie() and
sctp_unpack_cookie(), which seems to run from software interrupts.
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH V2] Fix a sleep-in-atomic bug in shash_setkey_unaligned
@ 2017-10-05 4:37 ` David Miller
0 siblings, 0 replies; 29+ messages in thread
From: David Miller @ 2017-10-05 4:37 UTC (permalink / raw)
To: herbert-lOAM2aK0SrRLBo1qDEOMRrpzq4S04n8Q
Cc: marcelo.leitner-Re5JQEeQqe8AvxtiuMwx3w,
luto-DgEjT+Ai2ygdnm+yROfE0A, baijiaju1990-9Onoh4P/yGk,
nhorman-2XuSBdqkA4R54TAoqtyWWQ, vyasevich-Re5JQEeQqe8AvxtiuMwx3w,
kvalo-sgV2jX0FEOL9JmXXK+q4OQ,
linux-crypto-u79uwXL29TY76Z2rM5mHXA,
netdev-u79uwXL29TY76Z2rM5mHXA, linux-sctp-u79uwXL29TY76Z2rM5mHXA,
linux-wireless-u79uwXL29TY76Z2rM5mHXA
From: Herbert Xu <herbert-lOAM2aK0SrRLBo1qDEOMRrpzq4S04n8Q@public.gmane.org>
Date: Thu, 5 Oct 2017 11:40:54 +0800
> On Tue, Oct 03, 2017 at 07:45:06PM -0300, Marcelo Ricardo Leitner wrote:
>>
>> > Usually if you're invoking setkey from a non-sleeping code-path
>> > you're probably doing something wrong.
>>
>> Usually but not always. There are 3 calls to that function on SCTP
>> code:
>> - pack a cookie, which is sent on an INIT_ACK packet to the client
>> - unpack the cookie above, after it is sent back by the client on a
>> COOKIE_ECHO packet
>> - send a chunk authenticated by a hash
>
> I'm not talking about the code-path in question. I'm talking
> about the function which generates the secret key in the first
> place. AFAICS that's only called in GFP_KERNEL context. What
> am I missing?
The setkey happens in functions like sctp_pack_cookie() and
sctp_unpack_cookie(), which seems to run from software interrupts.
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH V2] Fix a sleep-in-atomic bug in shash_setkey_unaligned
2017-10-05 4:37 ` David Miller
@ 2017-10-05 10:16 ` Herbert Xu
-1 siblings, 0 replies; 29+ messages in thread
From: Herbert Xu @ 2017-10-05 10:16 UTC (permalink / raw)
To: David Miller
Cc: marcelo.leitner, luto, baijiaju1990, nhorman, vyasevich, kvalo,
linux-crypto, netdev, linux-sctp, linux-wireless
On Wed, Oct 04, 2017 at 09:37:58PM -0700, David Miller wrote:
>
> > I'm not talking about the code-path in question. I'm talking
> > about the function which generates the secret key in the first
> > place. AFAICS that's only called in GFP_KERNEL context. What
> > am I missing?
>
> The setkey happens in functions like sctp_pack_cookie() and
> sctp_unpack_cookie(), which seems to run from software interrupts.
That was my point. Functions like sctp_pack_cookie shouldn't be
setting the key in the first place. The setkey should happen at
the point when the key is generated. That's sctp_endpoint_init
which AFAICS only gets called in GFP_KERNEL context.
Or is there a code-path where sctp_endpoint_init is called in
softirq context?
Cheers,
--
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] 29+ messages in thread
* Re: [PATCH V2] Fix a sleep-in-atomic bug in shash_setkey_unaligned
@ 2017-10-05 10:16 ` Herbert Xu
0 siblings, 0 replies; 29+ messages in thread
From: Herbert Xu @ 2017-10-05 10:16 UTC (permalink / raw)
To: David Miller
Cc: marcelo.leitner, luto, baijiaju1990, nhorman, vyasevich, kvalo,
linux-crypto, netdev, linux-sctp, linux-wireless
On Wed, Oct 04, 2017 at 09:37:58PM -0700, David Miller wrote:
>
> > I'm not talking about the code-path in question. I'm talking
> > about the function which generates the secret key in the first
> > place. AFAICS that's only called in GFP_KERNEL context. What
> > am I missing?
>
> The setkey happens in functions like sctp_pack_cookie() and
> sctp_unpack_cookie(), which seems to run from software interrupts.
That was my point. Functions like sctp_pack_cookie shouldn't be
setting the key in the first place. The setkey should happen at
the point when the key is generated. That's sctp_endpoint_init
which AFAICS only gets called in GFP_KERNEL context.
Or is there a code-path where sctp_endpoint_init is called in
softirq context?
Cheers,
--
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] 29+ messages in thread
* Re: [PATCH V2] Fix a sleep-in-atomic bug in shash_setkey_unaligned
@ 2017-10-05 13:16 ` Herbert Xu
0 siblings, 0 replies; 29+ messages in thread
From: Herbert Xu @ 2017-10-05 13:16 UTC (permalink / raw)
To: David Miller
Cc: marcelo.leitner, luto, baijiaju1990, nhorman, vyasevich, kvalo,
linux-crypto, netdev, linux-sctp, linux-wireless
On Thu, Oct 05, 2017 at 06:16:20PM +0800, Herbert Xu wrote:
>
> That was my point. Functions like sctp_pack_cookie shouldn't be
> setting the key in the first place. The setkey should happen at
> the point when the key is generated. That's sctp_endpoint_init
> which AFAICS only gets called in GFP_KERNEL context.
>
> Or is there a code-path where sctp_endpoint_init is called in
> softirq context?
OK, there are indeed code paths where the key is derived in softirq
context. Notably sctp_auth_calculate_hmac.
So I think this patch is the correct fix and I will push it upstream
as well as back to stable.
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] 29+ messages in thread
* Re: [PATCH V2] Fix a sleep-in-atomic bug in shash_setkey_unaligned
@ 2017-10-05 13:16 ` Herbert Xu
0 siblings, 0 replies; 29+ messages in thread
From: Herbert Xu @ 2017-10-05 13:16 UTC (permalink / raw)
To: David Miller
Cc: marcelo.leitner-Re5JQEeQqe8AvxtiuMwx3w,
luto-DgEjT+Ai2ygdnm+yROfE0A, baijiaju1990-9Onoh4P/yGk,
nhorman-2XuSBdqkA4R54TAoqtyWWQ, vyasevich-Re5JQEeQqe8AvxtiuMwx3w,
kvalo-sgV2jX0FEOL9JmXXK+q4OQ,
linux-crypto-u79uwXL29TY76Z2rM5mHXA,
netdev-u79uwXL29TY76Z2rM5mHXA, linux-sctp-u79uwXL29TY76Z2rM5mHXA,
linux-wireless-u79uwXL29TY76Z2rM5mHXA
On Thu, Oct 05, 2017 at 06:16:20PM +0800, Herbert Xu wrote:
>
> That was my point. Functions like sctp_pack_cookie shouldn't be
> setting the key in the first place. The setkey should happen at
> the point when the key is generated. That's sctp_endpoint_init
> which AFAICS only gets called in GFP_KERNEL context.
>
> Or is there a code-path where sctp_endpoint_init is called in
> softirq context?
OK, there are indeed code paths where the key is derived in softirq
context. Notably sctp_auth_calculate_hmac.
So I think this patch is the correct fix and I will push it upstream
as well as back to stable.
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] 29+ messages in thread
* Re: [PATCH V2] Fix a sleep-in-atomic bug in shash_setkey_unaligned
@ 2017-10-05 13:16 ` Herbert Xu
0 siblings, 0 replies; 29+ messages in thread
From: Herbert Xu @ 2017-10-05 13:16 UTC (permalink / raw)
To: David Miller
Cc: marcelo.leitner-Re5JQEeQqe8AvxtiuMwx3w,
luto-DgEjT+Ai2ygdnm+yROfE0A, baijiaju1990-9Onoh4P/yGk,
nhorman-2XuSBdqkA4R54TAoqtyWWQ, vyasevich-Re5JQEeQqe8AvxtiuMwx3w,
kvalo-sgV2jX0FEOL9JmXXK+q4OQ,
linux-crypto-u79uwXL29TY76Z2rM5mHXA,
netdev-u79uwXL29TY76Z2rM5mHXA, linux-sctp-u79uwXL29TY76Z2rM5mHXA,
linux-wireless-u79uwXL29TY76Z2rM5mHXA
On Thu, Oct 05, 2017 at 06:16:20PM +0800, Herbert Xu wrote:
>
> That was my point. Functions like sctp_pack_cookie shouldn't be
> setting the key in the first place. The setkey should happen at
> the point when the key is generated. That's sctp_endpoint_init
> which AFAICS only gets called in GFP_KERNEL context.
>
> Or is there a code-path where sctp_endpoint_init is called in
> softirq context?
OK, there are indeed code paths where the key is derived in softirq
context. Notably sctp_auth_calculate_hmac.
So I think this patch is the correct fix and I will push it upstream
as well as back to stable.
Thanks,
--
Email: Herbert Xu <herbert-lOAM2aK0SrRLBo1qDEOMRrpzq4S04n8Q@public.gmane.org>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH V2] Fix a sleep-in-atomic bug in shash_setkey_unaligned
2017-10-05 13:16 ` Herbert Xu
@ 2017-10-05 19:07 ` Marcelo Ricardo Leitner
-1 siblings, 0 replies; 29+ messages in thread
From: Marcelo Ricardo Leitner @ 2017-10-05 19:07 UTC (permalink / raw)
To: Herbert Xu
Cc: David Miller, luto, baijiaju1990, nhorman, vyasevich, kvalo,
linux-crypto, netdev, linux-sctp, linux-wireless
On Thu, Oct 05, 2017 at 09:16:31PM +0800, Herbert Xu wrote:
> On Thu, Oct 05, 2017 at 06:16:20PM +0800, Herbert Xu wrote:
> >
> > That was my point. Functions like sctp_pack_cookie shouldn't be
> > setting the key in the first place. The setkey should happen at
> > the point when the key is generated. That's sctp_endpoint_init
> > which AFAICS only gets called in GFP_KERNEL context.
> >
> > Or is there a code-path where sctp_endpoint_init is called in
> > softirq context?
>
> OK, there are indeed code paths where the key is derived in softirq
> context. Notably sctp_auth_calculate_hmac.
>
> So I think this patch is the correct fix and I will push it upstream
> as well as back to stable.
Okay, thanks.
Marcelo
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH V2] Fix a sleep-in-atomic bug in shash_setkey_unaligned
@ 2017-10-05 19:07 ` Marcelo Ricardo Leitner
0 siblings, 0 replies; 29+ messages in thread
From: Marcelo Ricardo Leitner @ 2017-10-05 19:07 UTC (permalink / raw)
To: Herbert Xu
Cc: David Miller, luto, baijiaju1990, nhorman, vyasevich, kvalo,
linux-crypto, netdev, linux-sctp, linux-wireless
On Thu, Oct 05, 2017 at 09:16:31PM +0800, Herbert Xu wrote:
> On Thu, Oct 05, 2017 at 06:16:20PM +0800, Herbert Xu wrote:
> >
> > That was my point. Functions like sctp_pack_cookie shouldn't be
> > setting the key in the first place. The setkey should happen at
> > the point when the key is generated. That's sctp_endpoint_init
> > which AFAICS only gets called in GFP_KERNEL context.
> >
> > Or is there a code-path where sctp_endpoint_init is called in
> > softirq context?
>
> OK, there are indeed code paths where the key is derived in softirq
> context. Notably sctp_auth_calculate_hmac.
>
> So I think this patch is the correct fix and I will push it upstream
> as well as back to stable.
Okay, thanks.
Marcelo
^ permalink raw reply [flat|nested] 29+ messages in thread