| Message ID | 20180620190408.45104-12-keescook@chromium.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Herbert Xu |
| Headers | show |
On Wed, Jun 20, 2018 at 9:04 PM, Kees Cook <keescook@chromium.org> wrote: > In the quest to remove all stack VLA usage from the kernel[1], this > caps the skcipher request size similar to other limits and adds a sanity > check at registration. > > > +#define SKCIPHER_MAX_REQSIZE (PAGE_SIZE / 8) > + > #define SKCIPHER_REQUEST_ON_STACK(name, tfm) \ > char __##name##_desc[sizeof(struct skcipher_request) + \ > - crypto_skcipher_reqsize(tfm)] CRYPTO_MINALIGN_ATTR; \ > + SKCIPHER_MAX_REQSIZE] CRYPTO_MINALIGN_ATTR; \ > struct skcipher_request *name = (void *)__##name##_desc > This is probably a bad idea on kernels with large values of PAGE_SIZE. Some users on ppc64 and arm64 use 64KB here, but still limit the per-function stack size to 2KB. Arnd
On 06/20/2018 07:04 PM, Kees Cook wrote: > In the quest to remove all stack VLA usage from the kernel[1], this > caps the skcipher request size similar to other limits and adds a sanity > check at registration. > > [1] https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qPXydAacU1RqZWA@mail.gmail.com > > Signed-off-by: Kees Cook <keescook@chromium.org> crypto/echainiv.c: In function ‘echainiv_encrypt’: crypto/echainiv.c:88:1: warning: the frame size of 2120 bytes is larger than 1024 bytes [-Wframe-larger-than=] crypto/authenc.c: In function ‘crypto_authenc_copy_assoc’: crypto/authenc.c:197:1: warning: the frame size of 2120 bytes is larger than 1024 bytes [-Wframe-larger-than=] crypto/authencesn.c: In function ‘crypto_authenc_esn_copy’: crypto/authencesn.c:194:1: warning: the frame size of 2120 bytes is larger than 1024 bytes [-Wframe-larger-than=] crypto/algif_aead.c: In function ‘crypto_aead_copy_sgl’: crypto/algif_aead.c:90:1: warning: the frame size of 2120 bytes is larger than 1024 bytes [-Wframe-larger-than=] Christophe > --- > include/crypto/internal/skcipher.h | 1 + > include/crypto/skcipher.h | 4 +++- > 2 files changed, 4 insertions(+), 1 deletion(-) > > diff --git a/include/crypto/internal/skcipher.h b/include/crypto/internal/skcipher.h > index e42f7063f245..5035482cbe68 100644 > --- a/include/crypto/internal/skcipher.h > +++ b/include/crypto/internal/skcipher.h > @@ -130,6 +130,7 @@ static inline struct crypto_skcipher *crypto_spawn_skcipher( > static inline void crypto_skcipher_set_reqsize( > struct crypto_skcipher *skcipher, unsigned int reqsize) > { > + BUG_ON(reqsize > SKCIPHER_MAX_REQSIZE); > skcipher->reqsize = reqsize; > } > > diff --git a/include/crypto/skcipher.h b/include/crypto/skcipher.h > index 2f327f090c3e..25294d0089b2 100644 > --- a/include/crypto/skcipher.h > +++ b/include/crypto/skcipher.h > @@ -139,9 +139,11 @@ struct skcipher_alg { > struct crypto_alg base; > }; > > +#define SKCIPHER_MAX_REQSIZE (PAGE_SIZE / 8) > + > #define SKCIPHER_REQUEST_ON_STACK(name, tfm) \ > char __##name##_desc[sizeof(struct skcipher_request) + \ > - crypto_skcipher_reqsize(tfm)] CRYPTO_MINALIGN_ATTR; \ > + SKCIPHER_MAX_REQSIZE] CRYPTO_MINALIGN_ATTR; \ > struct skcipher_request *name = (void *)__##name##_desc > > /** >
On Wed, Jun 20, 2018 at 12:44 PM, Arnd Bergmann <arnd@arndb.de> wrote: > On Wed, Jun 20, 2018 at 9:04 PM, Kees Cook <keescook@chromium.org> wrote: >> In the quest to remove all stack VLA usage from the kernel[1], this >> caps the skcipher request size similar to other limits and adds a sanity >> check at registration. >> >> >> +#define SKCIPHER_MAX_REQSIZE (PAGE_SIZE / 8) >> + >> #define SKCIPHER_REQUEST_ON_STACK(name, tfm) \ >> char __##name##_desc[sizeof(struct skcipher_request) + \ >> - crypto_skcipher_reqsize(tfm)] CRYPTO_MINALIGN_ATTR; \ >> + SKCIPHER_MAX_REQSIZE] CRYPTO_MINALIGN_ATTR; \ >> struct skcipher_request *name = (void *)__##name##_desc >> > > This is probably a bad idea on kernels with large values of PAGE_SIZE. > Some users on ppc64 and arm64 use 64KB here, but still limit > the per-function stack size to 2KB. We could make all of these PAGE_SIZE-related limits explicitly 512? I think that was the intent originally. -Kees
diff --git a/include/crypto/internal/skcipher.h b/include/crypto/internal/skcipher.h index e42f7063f245..5035482cbe68 100644 --- a/include/crypto/internal/skcipher.h +++ b/include/crypto/internal/skcipher.h @@ -130,6 +130,7 @@ static inline struct crypto_skcipher *crypto_spawn_skcipher( static inline void crypto_skcipher_set_reqsize( struct crypto_skcipher *skcipher, unsigned int reqsize) { + BUG_ON(reqsize > SKCIPHER_MAX_REQSIZE); skcipher->reqsize = reqsize; } diff --git a/include/crypto/skcipher.h b/include/crypto/skcipher.h index 2f327f090c3e..25294d0089b2 100644 --- a/include/crypto/skcipher.h +++ b/include/crypto/skcipher.h @@ -139,9 +139,11 @@ struct skcipher_alg { struct crypto_alg base; }; +#define SKCIPHER_MAX_REQSIZE (PAGE_SIZE / 8) + #define SKCIPHER_REQUEST_ON_STACK(name, tfm) \ char __##name##_desc[sizeof(struct skcipher_request) + \ - crypto_skcipher_reqsize(tfm)] CRYPTO_MINALIGN_ATTR; \ + SKCIPHER_MAX_REQSIZE] CRYPTO_MINALIGN_ATTR; \ struct skcipher_request *name = (void *)__##name##_desc /**
In the quest to remove all stack VLA usage from the kernel[1], this caps the skcipher request size similar to other limits and adds a sanity check at registration. [1] https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qPXydAacU1RqZWA@mail.gmail.com Signed-off-by: Kees Cook <keescook@chromium.org> --- include/crypto/internal/skcipher.h | 1 + include/crypto/skcipher.h | 4 +++- 2 files changed, 4 insertions(+), 1 deletion(-)