[v3,0/7] crypto: x86: Fix indirect function call casts
mbox series

Message ID 20190507161321.34611-1-keescook@chromium.org
Headers show
Series
  • crypto: x86: Fix indirect function call casts
Related show

Message

Kees Cook May 7, 2019, 4:13 p.m. UTC
It is possible to indirectly invoke functions with prototypes that do
not match those of the respectively used function pointers by using void
types or casts. This feature is frequently used as a way of relaxing
function invocation, making it possible that different data structures
are passed to different functions through the same pointer.

Despite the benefits, this can lead to a situation where functions with a
given prototype are invoked by pointers with a different prototype. This
is undesirable as it may prevent the use of heuristics such as prototype
matching-based Control-Flow Integrity, which can be used to prevent
ROP-based attacks.

One way of fixing this situation is through the use of inline helper
functions with prototypes that match the one in the respective invoking
pointer.

Given the above, the current efforts to improve the Linux security,
and the upcoming kernel support to compilers with CFI features, this
creates macros to be used to build the needed function definitions,
to be used in camellia, cast6, serpent, twofish, and aesni.

-Kees (and Joao)

v3:
- no longer RFC
- consolidate macros into glue_helper.h
- include aesni which was using casts as well
- remove XTS_TWEAK_CAST while we're at it

v2:
- update cast macros for clarity

v1:
- initial prototype

Joao Moreira (4):
  crypto: x86/crypto: Use new glue function macros
  crypto: x86/camellia: Use new glue function macros
  crypto: x86/twofish: Use new glue function macros
  crypto: x86/cast6: Use new glue function macros

Kees Cook (3):
  crypto: x86/glue_helper: Add static inline function glue macros
  crypto: x86/aesni: Use new glue function macros
  crypto: x86/glue_helper: Remove function prototype cast helpers

 arch/x86/crypto/aesni-intel_glue.c         | 31 ++++-----
 arch/x86/crypto/camellia_aesni_avx2_glue.c | 73 +++++++++-------------
 arch/x86/crypto/camellia_aesni_avx_glue.c  | 63 +++++++------------
 arch/x86/crypto/camellia_glue.c            | 21 +++----
 arch/x86/crypto/cast6_avx_glue.c           | 65 +++++++++----------
 arch/x86/crypto/serpent_avx2_glue.c        | 65 +++++++++----------
 arch/x86/crypto/serpent_avx_glue.c         | 58 ++++++-----------
 arch/x86/crypto/serpent_sse2_glue.c        | 27 +++++---
 arch/x86/crypto/twofish_avx_glue.c         | 71 ++++++++-------------
 arch/x86/crypto/twofish_glue_3way.c        | 28 ++++-----
 arch/x86/include/asm/crypto/camellia.h     | 64 ++++++-------------
 arch/x86/include/asm/crypto/glue_helper.h  | 34 ++++++++--
 arch/x86/include/asm/crypto/serpent-avx.h  | 28 ++++-----
 arch/x86/include/asm/crypto/twofish.h      | 22 ++++---
 include/crypto/xts.h                       |  2 -
 15 files changed, 283 insertions(+), 369 deletions(-)

Comments

Eric Biggers May 7, 2019, 5 p.m. UTC | #1
On Tue, May 07, 2019 at 09:13:14AM -0700, Kees Cook wrote:
> It is possible to indirectly invoke functions with prototypes that do
> not match those of the respectively used function pointers by using void
> types or casts. This feature is frequently used as a way of relaxing
> function invocation, making it possible that different data structures
> are passed to different functions through the same pointer.
> 
> Despite the benefits, this can lead to a situation where functions with a
> given prototype are invoked by pointers with a different prototype. This
> is undesirable as it may prevent the use of heuristics such as prototype
> matching-based Control-Flow Integrity, which can be used to prevent
> ROP-based attacks.
> 
> One way of fixing this situation is through the use of inline helper
> functions with prototypes that match the one in the respective invoking
> pointer.
> 
> Given the above, the current efforts to improve the Linux security,
> and the upcoming kernel support to compilers with CFI features, this
> creates macros to be used to build the needed function definitions,
> to be used in camellia, cast6, serpent, twofish, and aesni.

So why not change the function prototypes to be compatible with common_glue_*_t
instead, rather than wrapping them with another layer of functions?  Is it
because indirect calls into asm code won't be allowed with CFI?

> 
> -Kees (and Joao)
> 
> v3:
> - no longer RFC
> - consolidate macros into glue_helper.h
> - include aesni which was using casts as well
> - remove XTS_TWEAK_CAST while we're at it
> 
> v2:
> - update cast macros for clarity
> 
> v1:
> - initial prototype
> 
> Joao Moreira (4):
>   crypto: x86/crypto: Use new glue function macros

This one should be "x86/serpent", not "x86/crypto".

>   crypto: x86/camellia: Use new glue function macros
>   crypto: x86/twofish: Use new glue function macros
>   crypto: x86/cast6: Use new glue function macros
> 
> Kees Cook (3):
>   crypto: x86/glue_helper: Add static inline function glue macros
>   crypto: x86/aesni: Use new glue function macros
>   crypto: x86/glue_helper: Remove function prototype cast helpers
> 
>  arch/x86/crypto/aesni-intel_glue.c         | 31 ++++-----
>  arch/x86/crypto/camellia_aesni_avx2_glue.c | 73 +++++++++-------------
>  arch/x86/crypto/camellia_aesni_avx_glue.c  | 63 +++++++------------
>  arch/x86/crypto/camellia_glue.c            | 21 +++----
>  arch/x86/crypto/cast6_avx_glue.c           | 65 +++++++++----------
>  arch/x86/crypto/serpent_avx2_glue.c        | 65 +++++++++----------
>  arch/x86/crypto/serpent_avx_glue.c         | 58 ++++++-----------
>  arch/x86/crypto/serpent_sse2_glue.c        | 27 +++++---
>  arch/x86/crypto/twofish_avx_glue.c         | 71 ++++++++-------------
>  arch/x86/crypto/twofish_glue_3way.c        | 28 ++++-----
>  arch/x86/include/asm/crypto/camellia.h     | 64 ++++++-------------
>  arch/x86/include/asm/crypto/glue_helper.h  | 34 ++++++++--
>  arch/x86/include/asm/crypto/serpent-avx.h  | 28 ++++-----
>  arch/x86/include/asm/crypto/twofish.h      | 22 ++++---
>  include/crypto/xts.h                       |  2 -
>  15 files changed, 283 insertions(+), 369 deletions(-)
> 
> -- 
> 2.17.1
>
Kees Cook May 7, 2019, 9:07 p.m. UTC | #2
On Tue, May 7, 2019 at 10:00 AM Eric Biggers <ebiggers@kernel.org> wrote:
> > Given the above, the current efforts to improve the Linux security,
> > and the upcoming kernel support to compilers with CFI features, this
> > creates macros to be used to build the needed function definitions,
> > to be used in camellia, cast6, serpent, twofish, and aesni.
>
> So why not change the function prototypes to be compatible with common_glue_*_t
> instead, rather than wrapping them with another layer of functions?  Is it
> because indirect calls into asm code won't be allowed with CFI?

I don't know why they're not that way to begin with. But given that
the casting was already happening, this is just moving it to a place
where CFI won't be angry. :)

> >   crypto: x86/crypto: Use new glue function macros
>
> This one should be "x86/serpent", not "x86/crypto".

Oops, yes, that's my typo. I'll fix for v4. Do the conversions
themselves look okay (the changes are pretty mechanical)? If so,
Herbert, do you want a v4 with the typo fix, or do you want to fix
that up yourself?

Thanks!
Eric Biggers May 7, 2019, 9:50 p.m. UTC | #3
On Tue, May 07, 2019 at 02:07:51PM -0700, Kees Cook wrote:
> On Tue, May 7, 2019 at 10:00 AM Eric Biggers <ebiggers@kernel.org> wrote:
> > > Given the above, the current efforts to improve the Linux security,
> > > and the upcoming kernel support to compilers with CFI features, this
> > > creates macros to be used to build the needed function definitions,
> > > to be used in camellia, cast6, serpent, twofish, and aesni.
> >
> > So why not change the function prototypes to be compatible with common_glue_*_t
> > instead, rather than wrapping them with another layer of functions?  Is it
> > because indirect calls into asm code won't be allowed with CFI?
> 
> I don't know why they're not that way to begin with. But given that
> the casting was already happening, this is just moving it to a place
> where CFI won't be angry. :)
> 
> > >   crypto: x86/crypto: Use new glue function macros
> >
> > This one should be "x86/serpent", not "x86/crypto".
> 
> Oops, yes, that's my typo. I'll fix for v4. Do the conversions
> themselves look okay (the changes are pretty mechanical)? If so,
> Herbert, do you want a v4 with the typo fix, or do you want to fix
> that up yourself?
> 
> Thanks!
> 

I don't know yet.  It's difficult to read the code with 2 layers of macros.

Hence why I asked why you didn't just change the prototypes to be compatible.

- Eric
Herbert Xu May 8, 2019, 1:36 p.m. UTC | #4
On Tue, May 07, 2019 at 02:50:46PM -0700, Eric Biggers wrote:
>
> I don't know yet.  It's difficult to read the code with 2 layers of macros.
> 
> Hence why I asked why you didn't just change the prototypes to be compatible.

I agree.  Kees, since you're changing this anyway please make it
look better not worse.

Thanks,
Kees Cook May 8, 2019, 9:08 p.m. UTC | #5
On Wed, May 8, 2019 at 6:36 AM Herbert Xu <herbert@gondor.apana.org.au> wrote:
> On Tue, May 07, 2019 at 02:50:46PM -0700, Eric Biggers wrote:
> >
> > I don't know yet.  It's difficult to read the code with 2 layers of macros.
> >
> > Hence why I asked why you didn't just change the prototypes to be compatible.
>
> I agree.  Kees, since you're changing this anyway please make it
> look better not worse.

Do you mean I should use the typedefs in the new macros? I'm not aware
of a way to use a typedef to declare a function body, so I had to
repeat them. I'm open to suggestions!

As far as "fixing the prototypes", the API is agnostic of the context
type, and uses void *. And also it provides a way to call the same
function with different pointer types on the other arguments:

For example, quoting the existing code:

asmlinkage void twofish_dec_blk(struct twofish_ctx *ctx, u8 *dst,
                                const u8 *src);

Which is used for ecb and cbc:

#define GLUE_FUNC_CAST(fn) ((common_glue_func_t)(fn))
#define GLUE_CBC_FUNC_CAST(fn) ((common_glue_cbc_func_t)(fn))
...
static const struct common_glue_ctx twofish_dec = {
...
                .fn_u = { .ecb = GLUE_FUNC_CAST(twofish_dec_blk) }

static const struct common_glue_ctx twofish_dec_cbc = {
...
                .fn_u = { .cbc = GLUE_CBC_FUNC_CAST(twofish_dec_blk) }

which have different prototypes:

typedef void (*common_glue_func_t)(void *ctx, u8 *dst, const u8 *src);
typedef void (*common_glue_cbc_func_t)(void *ctx, u128 *dst, const u128 *src);
...
struct common_glue_func_entry {
        unsigned int num_blocks; /* number of blocks that @fn will process */
        union {
                common_glue_func_t ecb;
                common_glue_cbc_func_t cbc;
                common_glue_ctr_func_t ctr;
                common_glue_xts_func_t xts;
        } fn_u;
};

What CFI dislikes is calling a func(void *ctx, ...) when the actual
function is, for example, func(struct twofish_ctx *ctx, ...).

This needs to be fixed at the call site, not the static initializers,
and since the call site is void, there needs to be a static inline
that will satisfy the types.

I'm open to suggestions! :)

Thanks,
Herbert Xu May 9, 2019, 1:39 a.m. UTC | #6
On Wed, May 08, 2019 at 02:08:25PM -0700, Kees Cook wrote:
>
> For example, quoting the existing code:
> 
> asmlinkage void twofish_dec_blk(struct twofish_ctx *ctx, u8 *dst,
>                                 const u8 *src);

So just make it

	asmlinkage void twofish_dec_blk(void *ctx, u8 *dst, const u8 *src);

and you won't need any of these casts.

Cheers,
Eric Biggers May 9, 2019, 1:53 a.m. UTC | #7
On Tue, May 07, 2019 at 09:13:14AM -0700, Kees Cook wrote:
> It is possible to indirectly invoke functions with prototypes that do
> not match those of the respectively used function pointers by using void
> types or casts. This feature is frequently used as a way of relaxing
> function invocation, making it possible that different data structures
> are passed to different functions through the same pointer.
> 
> Despite the benefits, this can lead to a situation where functions with a
> given prototype are invoked by pointers with a different prototype. This
> is undesirable as it may prevent the use of heuristics such as prototype
> matching-based Control-Flow Integrity, which can be used to prevent
> ROP-based attacks.
> 
> One way of fixing this situation is through the use of inline helper
> functions with prototypes that match the one in the respective invoking
> pointer.
> 
> Given the above, the current efforts to improve the Linux security,
> and the upcoming kernel support to compilers with CFI features, this
> creates macros to be used to build the needed function definitions,
> to be used in camellia, cast6, serpent, twofish, and aesni.
> 
> -Kees (and Joao)

Did you try enabling -Wcast-function-type?  It seems you missed some cases:

arch/x86/crypto/sha256_ssse3_glue.c: In function ‘sha256_update’:
arch/x86/crypto/sha256_ssse3_glue.c:62:10: warning: cast between incompatible function types from ‘void (*)(u32 *, const char *, u64)’ {aka ‘void (*)(unsigned int *, const char *, long long unsigned int)’} to ‘void (*)(struct sha256_state *, const u8 *, int)’ {aka ‘void (*)(struct sha256_state *, const unsigned char *, int)’} [-Wcast-function-type]
          (sha256_block_fn *)sha256_xform);
          ^
arch/x86/crypto/sha256_ssse3_glue.c: In function ‘sha256_finup’:
arch/x86/crypto/sha256_ssse3_glue.c:77:11: warning: cast between incompatible function types from ‘void (*)(u32 *, const char *, u64)’ {aka ‘void (*)(unsigned int *, const char *, long long unsigned int)’} to ‘void (*)(struct sha256_state *, const u8 *, int)’ {aka ‘void (*)(struct sha256_state *, const unsigned char *, int)’} [-Wcast-function-type]
           (sha256_block_fn *)sha256_xform);
           ^
arch/x86/crypto/sha256_ssse3_glue.c:78:32: warning: cast between incompatible function types from ‘void (*)(u32 *, const char *, u64)’ {aka ‘void (*)(unsigned int *, const char *, long long unsigned int)’} to ‘void (*)(struct sha256_state *, const u8 *, int)’ {aka ‘void (*)(struct sha256_state *, const unsigned char *, int)’} [-Wcast-function-type]
  sha256_base_do_finalize(desc, (sha256_block_fn *)sha256_xform);
                                ^
  CC      arch/x86/crypto/sha512_ssse3_glue.o
arch/x86/crypto/sha512_ssse3_glue.c: In function ‘sha512_update’:
arch/x86/crypto/sha512_ssse3_glue.c:61:10: warning: cast between incompatible function types from ‘void (*)(u64 *, const char *, u64)’ {aka ‘void (*)(long long unsigned int *, const char *, long long unsigned int)’} to ‘void (*)(struct sha512_state *, const u8 *, int)’ {aka ‘void (*)(struct sha512_state *, const unsigned char *, int)’} [-Wcast-function-type]
          (sha512_block_fn *)sha512_xform);
          ^
arch/x86/crypto/sha512_ssse3_glue.c: In function ‘sha512_finup’:
arch/x86/crypto/sha512_ssse3_glue.c:76:11: warning: cast between incompatible function types from ‘void (*)(u64 *, const char *, u64)’ {aka ‘void (*)(long long unsigned int *, const char *, long long unsigned int)’} to ‘void (*)(struct sha512_state *, const u8 *, int)’ {aka ‘void (*)(struct sha512_state *, const unsigned char *, int)’} [-Wcast-function-type]
           (sha512_block_fn *)sha512_xform);
           ^
arch/x86/crypto/sha512_ssse3_glue.c:77:32: warning: cast between incompatible function types from ‘void (*)(u64 *, const char *, u64)’ {aka ‘void (*)(long long unsigned int *, const char *, long long unsigned int)’} to ‘void (*)(struct sha512_state *, const u8 *, int)’ {aka ‘void (*)(struct sha512_state *, const unsigned char *, int)’} [-Wcast-function-type]
  sha512_base_do_finalize(desc, (sha512_block_fn *)sha512_xform);
                                ^
Eric Biggers May 9, 2019, 2:04 a.m. UTC | #8
On Wed, May 08, 2019 at 02:08:25PM -0700, Kees Cook wrote:
> On Wed, May 8, 2019 at 6:36 AM Herbert Xu <herbert@gondor.apana.org.au> wrote:
> > On Tue, May 07, 2019 at 02:50:46PM -0700, Eric Biggers wrote:
> > >
> > > I don't know yet.  It's difficult to read the code with 2 layers of macros.
> > >
> > > Hence why I asked why you didn't just change the prototypes to be compatible.
> >
> > I agree.  Kees, since you're changing this anyway please make it
> > look better not worse.
> 
> Do you mean I should use the typedefs in the new macros? I'm not aware
> of a way to use a typedef to declare a function body, so I had to
> repeat them. I'm open to suggestions!
> 
> As far as "fixing the prototypes", the API is agnostic of the context
> type, and uses void *. And also it provides a way to call the same
> function with different pointer types on the other arguments:
> 
> For example, quoting the existing code:
> 
> asmlinkage void twofish_dec_blk(struct twofish_ctx *ctx, u8 *dst,
>                                 const u8 *src);
> 
> Which is used for ecb and cbc:
> 
> #define GLUE_FUNC_CAST(fn) ((common_glue_func_t)(fn))
> #define GLUE_CBC_FUNC_CAST(fn) ((common_glue_cbc_func_t)(fn))
> ...
> static const struct common_glue_ctx twofish_dec = {
> ...
>                 .fn_u = { .ecb = GLUE_FUNC_CAST(twofish_dec_blk) }
> 
> static const struct common_glue_ctx twofish_dec_cbc = {
> ...
>                 .fn_u = { .cbc = GLUE_CBC_FUNC_CAST(twofish_dec_blk) }
> 
> which have different prototypes:
> 
> typedef void (*common_glue_func_t)(void *ctx, u8 *dst, const u8 *src);
> typedef void (*common_glue_cbc_func_t)(void *ctx, u128 *dst, const u128 *src);
> ...
> struct common_glue_func_entry {
>         unsigned int num_blocks; /* number of blocks that @fn will process */
>         union {
>                 common_glue_func_t ecb;
>                 common_glue_cbc_func_t cbc;
>                 common_glue_ctr_func_t ctr;
>                 common_glue_xts_func_t xts;
>         } fn_u;
> };
> 

As Herbert said, the ctx parameters could be made 'void *'.

And I also asked whether indirect calls to asm code are even allowed with CFI.
IIRC, the AOSP kernels have been patched to remove them from arm64.  It would be
helpful if you would answer that question, since it would inform the best
approach here.

As for the "ecb" functions taking 'u8 *' but the "cbc" ones taking 'u128 *' and
the same function being used in the blocks==1 case, you could just pick one of
the types to use for both.  'u8 *' probably makes more sense since both ecb and
cbc operate on blocks of 16 bytes but don't interpret them as 128-bit integers.

- Eric
Joao Moreira May 9, 2019, 3:12 a.m. UTC | #9
On 5/8/19 11:04 PM, Eric Biggers wrote:
> On Wed, May 08, 2019 at 02:08:25PM -0700, Kees Cook wrote:
>> On Wed, May 8, 2019 at 6:36 AM Herbert Xu <herbert@gondor.apana.org.au> wrote:
>>> On Tue, May 07, 2019 at 02:50:46PM -0700, Eric Biggers wrote:
>>>>
>>>> I don't know yet.  It's difficult to read the code with 2 layers of macros.
>>>>
>>>> Hence why I asked why you didn't just change the prototypes to be compatible.
>>>
>>> I agree.  Kees, since you're changing this anyway please make it
>>> look better not worse.
>>
>> Do you mean I should use the typedefs in the new macros? I'm not aware
>> of a way to use a typedef to declare a function body, so I had to
>> repeat them. I'm open to suggestions!
>>
>> As far as "fixing the prototypes", the API is agnostic of the context
>> type, and uses void *. And also it provides a way to call the same
>> function with different pointer types on the other arguments:
>>
>> For example, quoting the existing code:
>>
>> asmlinkage void twofish_dec_blk(struct twofish_ctx *ctx, u8 *dst,
>>                                  const u8 *src);
>>
>> Which is used for ecb and cbc:
>>
>> #define GLUE_FUNC_CAST(fn) ((common_glue_func_t)(fn))
>> #define GLUE_CBC_FUNC_CAST(fn) ((common_glue_cbc_func_t)(fn))
>> ...
>> static const struct common_glue_ctx twofish_dec = {
>> ...
>>                  .fn_u = { .ecb = GLUE_FUNC_CAST(twofish_dec_blk) }
>>
>> static const struct common_glue_ctx twofish_dec_cbc = {
>> ...
>>                  .fn_u = { .cbc = GLUE_CBC_FUNC_CAST(twofish_dec_blk) }
>>
>> which have different prototypes:
>>
>> typedef void (*common_glue_func_t)(void *ctx, u8 *dst, const u8 *src);
>> typedef void (*common_glue_cbc_func_t)(void *ctx, u128 *dst, const u128 *src);
>> ...
>> struct common_glue_func_entry {
>>          unsigned int num_blocks; /* number of blocks that @fn will process */
>>          union {
>>                  common_glue_func_t ecb;
>>                  common_glue_cbc_func_t cbc;
>>                  common_glue_ctr_func_t ctr;
>>                  common_glue_xts_func_t xts;
>>          } fn_u;
>> };
>>
> 
> As Herbert said, the ctx parameters could be made 'void *'.
> 

This is how things were done in the original patch set, but some 
concerns were raised about this approach:

https://lkml.org/lkml/2018/4/16/74

Tks,
Joao.

> And I also asked whether indirect calls to asm code are even allowed with CFI.
> IIRC, the AOSP kernels have been patched to remove them from arm64.  It would be
> helpful if you would answer that question, since it would inform the best
> approach here.
> 
> As for the "ecb" functions taking 'u8 *' but the "cbc" ones taking 'u128 *' and
> the same function being used in the blocks==1 case, you could just pick one of
> the types to use for both.  'u8 *' probably makes more sense since both ecb and
> cbc operate on blocks of 16 bytes but don't interpret them as 128-bit integers.
> 
> - Eric
>
Herbert Xu May 9, 2019, 3:16 a.m. UTC | #10
On Thu, May 09, 2019 at 12:12:54AM -0300, Joao Moreira wrote:
>
> This is how things were done in the original patch set, but some concerns
> were raised about this approach:
> 
> https://lkml.org/lkml/2018/4/16/74

No that's not what I'm suggesting at all.  Just get rid of those
wrapper functions and change the underlying asm functions to take
a void *.

Cheers,
Sami Tolvanen May 9, 2019, 3:38 p.m. UTC | #11
On Wed, May 08, 2019 at 07:04:40PM -0700, Eric Biggers wrote:
> And I also asked whether indirect calls to asm code are even allowed
> with CFI. IIRC, the AOSP kernels have been patched to remove them from
> arm64

At least with clang, indirect calls to stand-alone assembly functions
trip CFI checks, which is why Android kernels use static inline stubs
to convert these to direct calls instead.

Sami
Eric Biggers May 9, 2019, 5:58 p.m. UTC | #12
On Thu, May 09, 2019 at 08:38:28AM -0700, Sami Tolvanen wrote:
> On Wed, May 08, 2019 at 07:04:40PM -0700, Eric Biggers wrote:
> > And I also asked whether indirect calls to asm code are even allowed
> > with CFI. IIRC, the AOSP kernels have been patched to remove them from
> > arm64
> 
> At least with clang, indirect calls to stand-alone assembly functions
> trip CFI checks, which is why Android kernels use static inline stubs
> to convert these to direct calls instead.
> 
> Sami

Thanks Sami.  Is there any way to annotate assembly functions such that they
work directly with CFI?  Otherwise, we need the wrapper functions.

Kees and Joao, it would be helpful if you'd explain this in the patchset.

- Eric
Sami Tolvanen May 9, 2019, 7:27 p.m. UTC | #13
On Thu, May 09, 2019 at 10:58:23AM -0700, Eric Biggers wrote:
> Is there any way to annotate assembly functions such that they work
> directly with CFI?

Not to my knowledge.

Sami