2020-03-18 22:47:41

by Kees Cook

[permalink] [raw]
Subject: Re: [PATCH v2 2/7] lib: prepare xxhash for preboot environment

On Tue, Mar 17, 2020 at 05:22:58PM -0700, Nick Terrell wrote:
> Don't export symbols if PREBOOT is defined.
>
> This change is necessary to get xxhash to work in a preboot environment,
> which is needed to support zstd-compressed kernels.

Hmm, I thought using this:

#define __DISABLE_EXPORTS

before the #include of the .c files solved this?

-Kees

>
> Signed-off-by: Nick Terrell <[email protected]>
> ---
> lib/xxhash.c | 21 ++++++++++++---------
> 1 file changed, 12 insertions(+), 9 deletions(-)
>
> diff --git a/lib/xxhash.c b/lib/xxhash.c
> index aa61e2a3802f..7f1d3cb01729 100644
> --- a/lib/xxhash.c
> +++ b/lib/xxhash.c
> @@ -80,13 +80,11 @@ void xxh32_copy_state(struct xxh32_state *dst, const struct xxh32_state *src)
> {
> memcpy(dst, src, sizeof(*dst));
> }
> -EXPORT_SYMBOL(xxh32_copy_state);
>
> void xxh64_copy_state(struct xxh64_state *dst, const struct xxh64_state *src)
> {
> memcpy(dst, src, sizeof(*dst));
> }
> -EXPORT_SYMBOL(xxh64_copy_state);
>
> /*-***************************
> * Simple Hash Functions
> @@ -151,7 +149,6 @@ uint32_t xxh32(const void *input, const size_t len, const uint32_t seed)
>
> return h32;
> }
> -EXPORT_SYMBOL(xxh32);
>
> static uint64_t xxh64_round(uint64_t acc, const uint64_t input)
> {
> @@ -234,7 +231,6 @@ uint64_t xxh64(const void *input, const size_t len, const uint64_t seed)
>
> return h64;
> }
> -EXPORT_SYMBOL(xxh64);
>
> /*-**************************************************
> * Advanced Hash Functions
> @@ -251,7 +247,6 @@ void xxh32_reset(struct xxh32_state *statePtr, const uint32_t seed)
> state.v4 = seed - PRIME32_1;
> memcpy(statePtr, &state, sizeof(state));
> }
> -EXPORT_SYMBOL(xxh32_reset);
>
> void xxh64_reset(struct xxh64_state *statePtr, const uint64_t seed)
> {
> @@ -265,7 +260,6 @@ void xxh64_reset(struct xxh64_state *statePtr, const uint64_t seed)
> state.v4 = seed - PRIME64_1;
> memcpy(statePtr, &state, sizeof(state));
> }
> -EXPORT_SYMBOL(xxh64_reset);
>
> int xxh32_update(struct xxh32_state *state, const void *input, const size_t len)
> {
> @@ -334,7 +328,6 @@ int xxh32_update(struct xxh32_state *state, const void *input, const size_t len)
>
> return 0;
> }
> -EXPORT_SYMBOL(xxh32_update);
>
> uint32_t xxh32_digest(const struct xxh32_state *state)
> {
> @@ -372,7 +365,6 @@ uint32_t xxh32_digest(const struct xxh32_state *state)
>
> return h32;
> }
> -EXPORT_SYMBOL(xxh32_digest);
>
> int xxh64_update(struct xxh64_state *state, const void *input, const size_t len)
> {
> @@ -439,7 +431,6 @@ int xxh64_update(struct xxh64_state *state, const void *input, const size_t len)
>
> return 0;
> }
> -EXPORT_SYMBOL(xxh64_update);
>
> uint64_t xxh64_digest(const struct xxh64_state *state)
> {
> @@ -494,7 +485,19 @@ uint64_t xxh64_digest(const struct xxh64_state *state)
>
> return h64;
> }
> +
> +#ifndef PREBOOT
> +EXPORT_SYMBOL(xxh32_copy_state);
> +EXPORT_SYMBOL(xxh64_copy_state);
> +EXPORT_SYMBOL(xxh32);
> +EXPORT_SYMBOL(xxh64);
> +EXPORT_SYMBOL(xxh32_reset);
> +EXPORT_SYMBOL(xxh64_reset);
> +EXPORT_SYMBOL(xxh32_update);
> +EXPORT_SYMBOL(xxh32_digest);
> +EXPORT_SYMBOL(xxh64_update);
> EXPORT_SYMBOL(xxh64_digest);
>
> MODULE_LICENSE("Dual BSD/GPL");
> MODULE_DESCRIPTION("xxHash");
> +#endif
> --
> 2.25.1
>

--
Kees Cook


2020-03-19 00:55:54

by Nick Terrell

[permalink] [raw]
Subject: Re: [PATCH v2 2/7] lib: prepare xxhash for preboot environment

> On Mar 18, 2020, at 3:45 PM, Kees Cook <[email protected]> wrote:
>
> On Tue, Mar 17, 2020 at 05:22:58PM -0700, Nick Terrell wrote:
>> Don't export symbols if PREBOOT is defined.
>>
>> This change is necessary to get xxhash to work in a preboot environment,
>> which is needed to support zstd-compressed kernels.
>
> Hmm, I thought using this:
>
> #define __DISABLE_EXPORTS
>
> before the #include of the .c files solved this?

I was able to get __DISABLE_EXPORTS to work. However, that still leaves the
MODULE_LICENSE() and MODULE_DESCRIPTION() in. I would prefer to use
the same method other decompressors use, because it is a well tested route and
debugging is hard in the preboot environment. If you feel strongly about it I will
change it.

I will plan on submitting a v3 tomorrow morning that fixes the style nits, and
splits PREBOOT into XXH_PREBOOT and ZSTD_PREBOOT.

Thanks for reviewing the patches!

-Nick

> -Kees
>
>>
>> Signed-off-by: Nick Terrell <[email protected]>
>> ---
>> lib/xxhash.c | 21 ++++++++++++---------
>> 1 file changed, 12 insertions(+), 9 deletions(-)
>>
>> diff --git a/lib/xxhash.c b/lib/xxhash.c
>> index aa61e2a3802f..7f1d3cb01729 100644
>> --- a/lib/xxhash.c
>> +++ b/lib/xxhash.c
>> @@ -80,13 +80,11 @@ void xxh32_copy_state(struct xxh32_state *dst, const struct xxh32_state *src)
>> {
>> memcpy(dst, src, sizeof(*dst));
>> }
>> -EXPORT_SYMBOL(xxh32_copy_state);
>>
>> void xxh64_copy_state(struct xxh64_state *dst, const struct xxh64_state *src)
>> {
>> memcpy(dst, src, sizeof(*dst));
>> }
>> -EXPORT_SYMBOL(xxh64_copy_state);
>>
>> /*-***************************
>> * Simple Hash Functions
>> @@ -151,7 +149,6 @@ uint32_t xxh32(const void *input, const size_t len, const uint32_t seed)
>>
>> return h32;
>> }
>> -EXPORT_SYMBOL(xxh32);
>>
>> static uint64_t xxh64_round(uint64_t acc, const uint64_t input)
>> {
>> @@ -234,7 +231,6 @@ uint64_t xxh64(const void *input, const size_t len, const uint64_t seed)
>>
>> return h64;
>> }
>> -EXPORT_SYMBOL(xxh64);
>>
>> /*-**************************************************
>> * Advanced Hash Functions
>> @@ -251,7 +247,6 @@ void xxh32_reset(struct xxh32_state *statePtr, const uint32_t seed)
>> state.v4 = seed - PRIME32_1;
>> memcpy(statePtr, &state, sizeof(state));
>> }
>> -EXPORT_SYMBOL(xxh32_reset);
>>
>> void xxh64_reset(struct xxh64_state *statePtr, const uint64_t seed)
>> {
>> @@ -265,7 +260,6 @@ void xxh64_reset(struct xxh64_state *statePtr, const uint64_t seed)
>> state.v4 = seed - PRIME64_1;
>> memcpy(statePtr, &state, sizeof(state));
>> }
>> -EXPORT_SYMBOL(xxh64_reset);
>>
>> int xxh32_update(struct xxh32_state *state, const void *input, const size_t len)
>> {
>> @@ -334,7 +328,6 @@ int xxh32_update(struct xxh32_state *state, const void *input, const size_t len)
>>
>> return 0;
>> }
>> -EXPORT_SYMBOL(xxh32_update);
>>
>> uint32_t xxh32_digest(const struct xxh32_state *state)
>> {
>> @@ -372,7 +365,6 @@ uint32_t xxh32_digest(const struct xxh32_state *state)
>>
>> return h32;
>> }
>> -EXPORT_SYMBOL(xxh32_digest);
>>
>> int xxh64_update(struct xxh64_state *state, const void *input, const size_t len)
>> {
>> @@ -439,7 +431,6 @@ int xxh64_update(struct xxh64_state *state, const void *input, const size_t len)
>>
>> return 0;
>> }
>> -EXPORT_SYMBOL(xxh64_update);
>>
>> uint64_t xxh64_digest(const struct xxh64_state *state)
>> {
>> @@ -494,7 +485,19 @@ uint64_t xxh64_digest(const struct xxh64_state *state)
>>
>> return h64;
>> }
>> +
>> +#ifndef PREBOOT
>> +EXPORT_SYMBOL(xxh32_copy_state);
>> +EXPORT_SYMBOL(xxh64_copy_state);
>> +EXPORT_SYMBOL(xxh32);
>> +EXPORT_SYMBOL(xxh64);
>> +EXPORT_SYMBOL(xxh32_reset);
>> +EXPORT_SYMBOL(xxh64_reset);
>> +EXPORT_SYMBOL(xxh32_update);
>> +EXPORT_SYMBOL(xxh32_digest);
>> +EXPORT_SYMBOL(xxh64_update);
>> EXPORT_SYMBOL(xxh64_digest);
>>
>> MODULE_LICENSE("Dual BSD/GPL");
>> MODULE_DESCRIPTION("xxHash");
>> +#endif
>> --
>> 2.25.1
>>
>
> --
> Kees Cook