* [PATCH] rxrpc: Reuse SKCIPHER_REQUEST_ON_STACK buffer
@ 2018-07-16 3:49 Kees Cook
2018-07-16 10:17 ` Arnd Bergmann
2018-07-16 17:42 ` Eric Biggers
0 siblings, 2 replies; 5+ messages in thread
From: Kees Cook @ 2018-07-16 3:49 UTC (permalink / raw)
To: David Howells
Cc: Herbert Xu, Arnd Bergmann, Eric Biggers, Gustavo A. R. Silva,
David S. Miller, netdev, linux-kernel
The use of SKCIPHER_REQUEST_ON_STACK() will trigger FRAME_WARN warnings
(when less than 2048) once the VLA is no longer hidden from the check:
net/rxrpc/rxkad.c:398:1: warning: the frame size of 1152 bytes is larger than 1024 bytes [-Wframe-larger-than=]
net/rxrpc/rxkad.c:242:1: warning: the frame size of 1152 bytes is larger than 1024 bytes [-Wframe-larger-than=]
This passes the initial SKCIPHER_REQUEST_ON_STACK allocation to the leaf
functions for reuse. Two requests allocated on the stack are not needed
when only one is used at a time.
Signed-off-by: Kees Cook <keescook@chromium.org>
---
net/rxrpc/rxkad.c | 25 +++++++++++++------------
1 file changed, 13 insertions(+), 12 deletions(-)
diff --git a/net/rxrpc/rxkad.c b/net/rxrpc/rxkad.c
index 278ac0807a60..6393391fac86 100644
--- a/net/rxrpc/rxkad.c
+++ b/net/rxrpc/rxkad.c
@@ -146,10 +146,10 @@ static int rxkad_prime_packet_security(struct rxrpc_connection *conn)
static int rxkad_secure_packet_auth(const struct rxrpc_call *call,
struct sk_buff *skb,
u32 data_size,
- void *sechdr)
+ void *sechdr,
+ struct skcipher_request *req)
{
struct rxrpc_skb_priv *sp = rxrpc_skb(skb);
- SKCIPHER_REQUEST_ON_STACK(req, call->conn->cipher);
struct rxkad_level1_hdr hdr;
struct rxrpc_crypt iv;
struct scatterlist sg;
@@ -183,12 +183,12 @@ static int rxkad_secure_packet_auth(const struct rxrpc_call *call,
static int rxkad_secure_packet_encrypt(const struct rxrpc_call *call,
struct sk_buff *skb,
u32 data_size,
- void *sechdr)
+ void *sechdr,
+ struct skcipher_request *req)
{
const struct rxrpc_key_token *token;
struct rxkad_level2_hdr rxkhdr;
struct rxrpc_skb_priv *sp;
- SKCIPHER_REQUEST_ON_STACK(req, call->conn->cipher);
struct rxrpc_crypt iv;
struct scatterlist sg[16];
struct sk_buff *trailer;
@@ -296,11 +296,12 @@ static int rxkad_secure_packet(struct rxrpc_call *call,
ret = 0;
break;
case RXRPC_SECURITY_AUTH:
- ret = rxkad_secure_packet_auth(call, skb, data_size, sechdr);
+ ret = rxkad_secure_packet_auth(call, skb, data_size, sechdr,
+ req);
break;
case RXRPC_SECURITY_ENCRYPT:
ret = rxkad_secure_packet_encrypt(call, skb, data_size,
- sechdr);
+ sechdr, req);
break;
default:
ret = -EPERM;
@@ -316,10 +317,10 @@ static int rxkad_secure_packet(struct rxrpc_call *call,
*/
static int rxkad_verify_packet_1(struct rxrpc_call *call, struct sk_buff *skb,
unsigned int offset, unsigned int len,
- rxrpc_seq_t seq)
+ rxrpc_seq_t seq,
+ struct skcipher_request *req)
{
struct rxkad_level1_hdr sechdr;
- SKCIPHER_REQUEST_ON_STACK(req, call->conn->cipher);
struct rxrpc_crypt iv;
struct scatterlist sg[16];
struct sk_buff *trailer;
@@ -402,11 +403,11 @@ static int rxkad_verify_packet_1(struct rxrpc_call *call, struct sk_buff *skb,
*/
static int rxkad_verify_packet_2(struct rxrpc_call *call, struct sk_buff *skb,
unsigned int offset, unsigned int len,
- rxrpc_seq_t seq)
+ rxrpc_seq_t seq,
+ struct skcipher_request *req)
{
const struct rxrpc_key_token *token;
struct rxkad_level2_hdr sechdr;
- SKCIPHER_REQUEST_ON_STACK(req, call->conn->cipher);
struct rxrpc_crypt iv;
struct scatterlist _sg[4], *sg;
struct sk_buff *trailer;
@@ -549,9 +550,9 @@ static int rxkad_verify_packet(struct rxrpc_call *call, struct sk_buff *skb,
case RXRPC_SECURITY_PLAIN:
return 0;
case RXRPC_SECURITY_AUTH:
- return rxkad_verify_packet_1(call, skb, offset, len, seq);
+ return rxkad_verify_packet_1(call, skb, offset, len, seq, req);
case RXRPC_SECURITY_ENCRYPT:
- return rxkad_verify_packet_2(call, skb, offset, len, seq);
+ return rxkad_verify_packet_2(call, skb, offset, len, seq, req);
default:
return -ENOANO;
}
--
2.17.1
--
Kees Cook
Pixel Security
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] rxrpc: Reuse SKCIPHER_REQUEST_ON_STACK buffer
2018-07-16 3:49 [PATCH] rxrpc: Reuse SKCIPHER_REQUEST_ON_STACK buffer Kees Cook
@ 2018-07-16 10:17 ` Arnd Bergmann
2018-07-19 4:29 ` Kees Cook
2018-07-16 17:42 ` Eric Biggers
1 sibling, 1 reply; 5+ messages in thread
From: Arnd Bergmann @ 2018-07-16 10:17 UTC (permalink / raw)
To: Kees Cook
Cc: David Howells, Herbert Xu, Eric Biggers, Gustavo A. R. Silva,
David S. Miller, Networking, Linux Kernel Mailing List
On Mon, Jul 16, 2018 at 5:49 AM, Kees Cook <keescook@chromium.org> wrote:
> The use of SKCIPHER_REQUEST_ON_STACK() will trigger FRAME_WARN warnings
> (when less than 2048) once the VLA is no longer hidden from the check:
>
> net/rxrpc/rxkad.c:398:1: warning: the frame size of 1152 bytes is larger than 1024 bytes [-Wframe-larger-than=]
> net/rxrpc/rxkad.c:242:1: warning: the frame size of 1152 bytes is larger than 1024 bytes [-Wframe-larger-than=]
>
> This passes the initial SKCIPHER_REQUEST_ON_STACK allocation to the leaf
> functions for reuse. Two requests allocated on the stack are not needed
> when only one is used at a time.
>
> Signed-off-by: Kees Cook <keescook@chromium.org>
This looks like a very nice solution to the problem.
Acked-by: Arnd Bergmann <arnd@arndb.de>
Since the large stack usage could already cause problems in older kernels,
should this be backported to stable kernels as well?
Arnd
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] rxrpc: Reuse SKCIPHER_REQUEST_ON_STACK buffer
2018-07-16 3:49 [PATCH] rxrpc: Reuse SKCIPHER_REQUEST_ON_STACK buffer Kees Cook
2018-07-16 10:17 ` Arnd Bergmann
@ 2018-07-16 17:42 ` Eric Biggers
2018-07-19 4:31 ` Kees Cook
1 sibling, 1 reply; 5+ messages in thread
From: Eric Biggers @ 2018-07-16 17:42 UTC (permalink / raw)
To: Kees Cook
Cc: David Howells, Herbert Xu, Arnd Bergmann, Gustavo A. R. Silva,
David S. Miller, netdev, linux-kernel
On Sun, Jul 15, 2018 at 08:49:47PM -0700, Kees Cook wrote:
> The use of SKCIPHER_REQUEST_ON_STACK() will trigger FRAME_WARN warnings
> (when less than 2048) once the VLA is no longer hidden from the check:
>
> net/rxrpc/rxkad.c:398:1: warning: the frame size of 1152 bytes is larger than 1024 bytes [-Wframe-larger-than=]
> net/rxrpc/rxkad.c:242:1: warning: the frame size of 1152 bytes is larger than 1024 bytes [-Wframe-larger-than=]
>
> This passes the initial SKCIPHER_REQUEST_ON_STACK allocation to the leaf
> functions for reuse. Two requests allocated on the stack are not needed
> when only one is used at a time.
>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
> net/rxrpc/rxkad.c | 25 +++++++++++++------------
> 1 file changed, 13 insertions(+), 12 deletions(-)
>
> diff --git a/net/rxrpc/rxkad.c b/net/rxrpc/rxkad.c
> index 278ac0807a60..6393391fac86 100644
> --- a/net/rxrpc/rxkad.c
> +++ b/net/rxrpc/rxkad.c
> @@ -146,10 +146,10 @@ static int rxkad_prime_packet_security(struct rxrpc_connection *conn)
> static int rxkad_secure_packet_auth(const struct rxrpc_call *call,
> struct sk_buff *skb,
> u32 data_size,
> - void *sechdr)
> + void *sechdr,
> + struct skcipher_request *req)
> {
> struct rxrpc_skb_priv *sp = rxrpc_skb(skb);
> - SKCIPHER_REQUEST_ON_STACK(req, call->conn->cipher);
> struct rxkad_level1_hdr hdr;
> struct rxrpc_crypt iv;
> struct scatterlist sg;
> @@ -183,12 +183,12 @@ static int rxkad_secure_packet_auth(const struct rxrpc_call *call,
> static int rxkad_secure_packet_encrypt(const struct rxrpc_call *call,
> struct sk_buff *skb,
> u32 data_size,
> - void *sechdr)
> + void *sechdr,
> + struct skcipher_request *req)
> {
> const struct rxrpc_key_token *token;
> struct rxkad_level2_hdr rxkhdr;
> struct rxrpc_skb_priv *sp;
> - SKCIPHER_REQUEST_ON_STACK(req, call->conn->cipher);
> struct rxrpc_crypt iv;
> struct scatterlist sg[16];
> struct sk_buff *trailer;
> @@ -296,11 +296,12 @@ static int rxkad_secure_packet(struct rxrpc_call *call,
> ret = 0;
> break;
> case RXRPC_SECURITY_AUTH:
> - ret = rxkad_secure_packet_auth(call, skb, data_size, sechdr);
> + ret = rxkad_secure_packet_auth(call, skb, data_size, sechdr,
> + req);
> break;
> case RXRPC_SECURITY_ENCRYPT:
> ret = rxkad_secure_packet_encrypt(call, skb, data_size,
> - sechdr);
> + sechdr, req);
> break;
> default:
> ret = -EPERM;
> @@ -316,10 +317,10 @@ static int rxkad_secure_packet(struct rxrpc_call *call,
> */
> static int rxkad_verify_packet_1(struct rxrpc_call *call, struct sk_buff *skb,
> unsigned int offset, unsigned int len,
> - rxrpc_seq_t seq)
> + rxrpc_seq_t seq,
> + struct skcipher_request *req)
> {
> struct rxkad_level1_hdr sechdr;
> - SKCIPHER_REQUEST_ON_STACK(req, call->conn->cipher);
> struct rxrpc_crypt iv;
> struct scatterlist sg[16];
> struct sk_buff *trailer;
> @@ -402,11 +403,11 @@ static int rxkad_verify_packet_1(struct rxrpc_call *call, struct sk_buff *skb,
> */
> static int rxkad_verify_packet_2(struct rxrpc_call *call, struct sk_buff *skb,
> unsigned int offset, unsigned int len,
> - rxrpc_seq_t seq)
> + rxrpc_seq_t seq,
> + struct skcipher_request *req)
> {
> const struct rxrpc_key_token *token;
> struct rxkad_level2_hdr sechdr;
> - SKCIPHER_REQUEST_ON_STACK(req, call->conn->cipher);
> struct rxrpc_crypt iv;
> struct scatterlist _sg[4], *sg;
> struct sk_buff *trailer;
> @@ -549,9 +550,9 @@ static int rxkad_verify_packet(struct rxrpc_call *call, struct sk_buff *skb,
> case RXRPC_SECURITY_PLAIN:
> return 0;
> case RXRPC_SECURITY_AUTH:
> - return rxkad_verify_packet_1(call, skb, offset, len, seq);
> + return rxkad_verify_packet_1(call, skb, offset, len, seq, req);
> case RXRPC_SECURITY_ENCRYPT:
> - return rxkad_verify_packet_2(call, skb, offset, len, seq);
> + return rxkad_verify_packet_2(call, skb, offset, len, seq, req);
> default:
> return -ENOANO;
> }
> --
> 2.17.1
How about doing the 'skcipher_request_set_tfm(req, call->conn->cipher)' and
'skcipher_request_zero(req);' just once, in the top-level function
rxkad_verify_packet(), instead of before/after every time the request is used?
- Eric
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] rxrpc: Reuse SKCIPHER_REQUEST_ON_STACK buffer
2018-07-16 10:17 ` Arnd Bergmann
@ 2018-07-19 4:29 ` Kees Cook
0 siblings, 0 replies; 5+ messages in thread
From: Kees Cook @ 2018-07-19 4:29 UTC (permalink / raw)
To: Arnd Bergmann
Cc: David Howells, Herbert Xu, Eric Biggers, Gustavo A. R. Silva,
David S. Miller, Networking, Linux Kernel Mailing List
On Mon, Jul 16, 2018 at 3:17 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Mon, Jul 16, 2018 at 5:49 AM, Kees Cook <keescook@chromium.org> wrote:
>> The use of SKCIPHER_REQUEST_ON_STACK() will trigger FRAME_WARN warnings
>> (when less than 2048) once the VLA is no longer hidden from the check:
>>
>> net/rxrpc/rxkad.c:398:1: warning: the frame size of 1152 bytes is larger than 1024 bytes [-Wframe-larger-than=]
>> net/rxrpc/rxkad.c:242:1: warning: the frame size of 1152 bytes is larger than 1024 bytes [-Wframe-larger-than=]
>>
>> This passes the initial SKCIPHER_REQUEST_ON_STACK allocation to the leaf
>> functions for reuse. Two requests allocated on the stack are not needed
>> when only one is used at a time.
>>
>> Signed-off-by: Kees Cook <keescook@chromium.org>
>
> This looks like a very nice solution to the problem.
>
> Acked-by: Arnd Bergmann <arnd@arndb.de>
>
> Since the large stack usage could already cause problems in older kernels,
> should this be backported to stable kernels as well?
I don't think this hits the bar for doing a -stable patch, but if you
think I should, I can do it.
-Kees
--
Kees Cook
Pixel Security
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] rxrpc: Reuse SKCIPHER_REQUEST_ON_STACK buffer
2018-07-16 17:42 ` Eric Biggers
@ 2018-07-19 4:31 ` Kees Cook
0 siblings, 0 replies; 5+ messages in thread
From: Kees Cook @ 2018-07-19 4:31 UTC (permalink / raw)
To: Eric Biggers
Cc: David Howells, Herbert Xu, Arnd Bergmann, Gustavo A. R. Silva,
David S. Miller, Network Development, LKML
On Mon, Jul 16, 2018 at 10:42 AM, Eric Biggers <ebiggers@google.com> wrote:
> On Sun, Jul 15, 2018 at 08:49:47PM -0700, Kees Cook wrote:
>> The use of SKCIPHER_REQUEST_ON_STACK() will trigger FRAME_WARN warnings
>> (when less than 2048) once the VLA is no longer hidden from the check:
>>
>> net/rxrpc/rxkad.c:398:1: warning: the frame size of 1152 bytes is larger than 1024 bytes [-Wframe-larger-than=]
>> net/rxrpc/rxkad.c:242:1: warning: the frame size of 1152 bytes is larger than 1024 bytes [-Wframe-larger-than=]
>>
>> This passes the initial SKCIPHER_REQUEST_ON_STACK allocation to the leaf
>> functions for reuse. Two requests allocated on the stack are not needed
>> when only one is used at a time.
>>
>> Signed-off-by: Kees Cook <keescook@chromium.org>
>> ---
>> net/rxrpc/rxkad.c | 25 +++++++++++++------------
>> 1 file changed, 13 insertions(+), 12 deletions(-)
>> [...]
> How about doing the 'skcipher_request_set_tfm(req, call->conn->cipher)' and
> 'skcipher_request_zero(req);' just once, in the top-level function
> rxkad_verify_packet(), instead of before/after every time the request is used?
Hm. While that does sound reasonable, I like having it be
operationally unchanged. And changing this makes the caller function a
bit more weird. Right now it's doing direct returns, and we'd need to
cover error paths, etc. Having the "zero" _right_ after the
encrypt/decrypt seems more robust to me.
-Kees
--
Kees Cook
Pixel Security
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-07-19 4:31 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-07-16 3:49 [PATCH] rxrpc: Reuse SKCIPHER_REQUEST_ON_STACK buffer Kees Cook
2018-07-16 10:17 ` Arnd Bergmann
2018-07-19 4:29 ` Kees Cook
2018-07-16 17:42 ` Eric Biggers
2018-07-19 4:31 ` Kees Cook
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).