On Fri, Nov 04, 2022 at 07:33:43PM +0800, kernel test robot wrote:
>
> vim +34 arch/arm/crypto/sha1_glue.c
>
> f0be44f4fb1fae David McCullough 2012-09-07 23
> 1f8673d31a999e Jussi Kivilinna 2014-07-29 24 asmlinkage void sha1_block_data_order(u32 *digest,
> f0be44f4fb1fae David McCullough 2012-09-07 25 const unsigned char *data, unsigned int rounds);
> f0be44f4fb1fae David McCullough 2012-09-07 26
> 604682551aa511 Jussi Kivilinna 2014-07-29 27 int sha1_update_arm(struct shash_desc *desc, const u8 *data,
> f0be44f4fb1fae David McCullough 2012-09-07 28 unsigned int len)
> f0be44f4fb1fae David McCullough 2012-09-07 29 {
> 90451d6bdb787e Ard Biesheuvel 2015-04-09 30 /* make sure casting to sha1_block_fn() is safe */
> 90451d6bdb787e Ard Biesheuvel 2015-04-09 31 BUILD_BUG_ON(offsetof(struct sha1_state, state) != 0);
> f0be44f4fb1fae David McCullough 2012-09-07 32
> 90451d6bdb787e Ard Biesheuvel 2015-04-09 33 return sha1_base_do_update(desc, data, len,
> 90451d6bdb787e Ard Biesheuvel 2015-04-09 @34 (sha1_block_fn *)sha1_block_data_order);
> f0be44f4fb1fae David McCullough 2012-09-07 35 }
> 604682551aa511 Jussi Kivilinna 2014-07-29 36 EXPORT_SYMBOL_GPL(sha1_update_arm);
> f0be44f4fb1fae David McCullough 2012-09-07 37
So clan doesn't like the cast on the assembly function.
Ard, why can't we just change the signature of the assembly
function instead of casting?
Thanks,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
On Thu, 8 Dec 2022 at 05:59, Herbert Xu <[email protected]> wrote:
>
> On Fri, Nov 04, 2022 at 07:33:43PM +0800, kernel test robot wrote:
> >
> > vim +34 arch/arm/crypto/sha1_glue.c
> >
> > f0be44f4fb1fae David McCullough 2012-09-07 23
> > 1f8673d31a999e Jussi Kivilinna 2014-07-29 24 asmlinkage void sha1_block_data_order(u32 *digest,
> > f0be44f4fb1fae David McCullough 2012-09-07 25 const unsigned char *data, unsigned int rounds);
> > f0be44f4fb1fae David McCullough 2012-09-07 26
> > 604682551aa511 Jussi Kivilinna 2014-07-29 27 int sha1_update_arm(struct shash_desc *desc, const u8 *data,
> > f0be44f4fb1fae David McCullough 2012-09-07 28 unsigned int len)
> > f0be44f4fb1fae David McCullough 2012-09-07 29 {
> > 90451d6bdb787e Ard Biesheuvel 2015-04-09 30 /* make sure casting to sha1_block_fn() is safe */
> > 90451d6bdb787e Ard Biesheuvel 2015-04-09 31 BUILD_BUG_ON(offsetof(struct sha1_state, state) != 0);
> > f0be44f4fb1fae David McCullough 2012-09-07 32
> > 90451d6bdb787e Ard Biesheuvel 2015-04-09 33 return sha1_base_do_update(desc, data, len,
> > 90451d6bdb787e Ard Biesheuvel 2015-04-09 @34 (sha1_block_fn *)sha1_block_data_order);
> > f0be44f4fb1fae David McCullough 2012-09-07 35 }
> > 604682551aa511 Jussi Kivilinna 2014-07-29 36 EXPORT_SYMBOL_GPL(sha1_update_arm);
> > f0be44f4fb1fae David McCullough 2012-09-07 37
>
> So clan doesn't like the cast on the assembly function.
>
> Ard, why can't we just change the signature of the assembly
> function instead of casting?
>
We can, as the BUILD_BUG() will catch it if struct sha1_state gets
modified in an incompatible way.
On Thu, Dec 08, 2022 at 07:05:45PM +0100, Ard Biesheuvel wrote:
>
> We can, as the BUILD_BUG() will catch it if struct sha1_state gets
> modified in an incompatible way.
Thanks for confirming!
---8<---
Instead of casting the function which upsets clang for some reason,
change the assembly function siganture instead.
Reported-by: kernel test robot <[email protected]>
Signed-off-by: Herbert Xu <[email protected]>
diff --git a/arch/arm/crypto/sha1_glue.c b/arch/arm/crypto/sha1_glue.c
index 6c2b849e459d..95a727bcd664 100644
--- a/arch/arm/crypto/sha1_glue.c
+++ b/arch/arm/crypto/sha1_glue.c
@@ -21,31 +21,29 @@
#include "sha1.h"
-asmlinkage void sha1_block_data_order(u32 *digest,
- const unsigned char *data, unsigned int rounds);
+asmlinkage void sha1_block_data_order(struct sha1_state *digest,
+ const u8 *data, int rounds);
int sha1_update_arm(struct shash_desc *desc, const u8 *data,
unsigned int len)
{
- /* make sure casting to sha1_block_fn() is safe */
+ /* make sure signature matches sha1_block_fn() */
BUILD_BUG_ON(offsetof(struct sha1_state, state) != 0);
- return sha1_base_do_update(desc, data, len,
- (sha1_block_fn *)sha1_block_data_order);
+ return sha1_base_do_update(desc, data, len, sha1_block_data_order);
}
EXPORT_SYMBOL_GPL(sha1_update_arm);
static int sha1_final(struct shash_desc *desc, u8 *out)
{
- sha1_base_do_finalize(desc, (sha1_block_fn *)sha1_block_data_order);
+ sha1_base_do_finalize(desc, sha1_block_data_order);
return sha1_base_finish(desc, out);
}
int sha1_finup_arm(struct shash_desc *desc, const u8 *data,
unsigned int len, u8 *out)
{
- sha1_base_do_update(desc, data, len,
- (sha1_block_fn *)sha1_block_data_order);
+ sha1_base_do_update(desc, data, len, sha1_block_data_order);
return sha1_final(desc, out);
}
EXPORT_SYMBOL_GPL(sha1_finup_arm);
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
On Tue, 13 Dec 2022 at 11:43, Herbert Xu <[email protected]> wrote:
>
> On Thu, Dec 08, 2022 at 07:05:45PM +0100, Ard Biesheuvel wrote:
> >
> > We can, as the BUILD_BUG() will catch it if struct sha1_state gets
> > modified in an incompatible way.
>
> Thanks for confirming!
>
> ---8<---
> Instead of casting the function which upsets clang for some reason,
> change the assembly function siganture instead.
>
> Reported-by: kernel test robot <[email protected]>
> Signed-off-by: Herbert Xu <[email protected]>
Reviewed-by: Ard Biesheuvel <[email protected]>
>
> diff --git a/arch/arm/crypto/sha1_glue.c b/arch/arm/crypto/sha1_glue.c
> index 6c2b849e459d..95a727bcd664 100644
> --- a/arch/arm/crypto/sha1_glue.c
> +++ b/arch/arm/crypto/sha1_glue.c
> @@ -21,31 +21,29 @@
>
> #include "sha1.h"
>
> -asmlinkage void sha1_block_data_order(u32 *digest,
> - const unsigned char *data, unsigned int rounds);
> +asmlinkage void sha1_block_data_order(struct sha1_state *digest,
> + const u8 *data, int rounds);
>
> int sha1_update_arm(struct shash_desc *desc, const u8 *data,
> unsigned int len)
> {
> - /* make sure casting to sha1_block_fn() is safe */
> + /* make sure signature matches sha1_block_fn() */
> BUILD_BUG_ON(offsetof(struct sha1_state, state) != 0);
>
> - return sha1_base_do_update(desc, data, len,
> - (sha1_block_fn *)sha1_block_data_order);
> + return sha1_base_do_update(desc, data, len, sha1_block_data_order);
> }
> EXPORT_SYMBOL_GPL(sha1_update_arm);
>
> static int sha1_final(struct shash_desc *desc, u8 *out)
> {
> - sha1_base_do_finalize(desc, (sha1_block_fn *)sha1_block_data_order);
> + sha1_base_do_finalize(desc, sha1_block_data_order);
> return sha1_base_finish(desc, out);
> }
>
> int sha1_finup_arm(struct shash_desc *desc, const u8 *data,
> unsigned int len, u8 *out)
> {
> - sha1_base_do_update(desc, data, len,
> - (sha1_block_fn *)sha1_block_data_order);
> + sha1_base_do_update(desc, data, len, sha1_block_data_order);
> return sha1_final(desc, out);
> }
> EXPORT_SYMBOL_GPL(sha1_finup_arm);
> --
> Email: Herbert Xu <[email protected]>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
> On Thu, Dec 08, 2022 at 07:05:45PM +0100, Ard Biesheuvel wrote:
...
> Instead of casting the function which upsets clang for some reason,
> change the assembly function siganture instead.
Since the assembly function is passed directly to the generic sha1 helper
(sha1_base_do_update) then I agree that is the right approach. The
casts are a clue that something is not quite right.
There are several other arm modules with casts, not just sha1:
sha1, sha1_neon, sha256, sha256_neon, sha2-ce, sha512, sha512-neon
> - /* make sure casting to sha1_block_fn() is safe */
> + /* make sure signature matches sha1_block_fn() */
> BUILD_BUG_ON(offsetof(struct sha1_state, state) != 0);
I think this wording used by some of the modules provides a better
explanation (e.g. arch/arm/sha512-neon-glue.c):
* Make sure struct sha512_state begins directly with the SHA512
* 512-bit internal state, as this is what the asm functions expect.
Most of them only access the first field in the structure, so I'd be
tempted to change the callers to pass the offset of the state subfield
and make the prototypes match that, changing:
block_fn(sctx, data, blocks);
to
block_fn(sctx->state, data, blocks);
However, a few of them (e.g., arm64 sha1_neon_ce) do carefully
manipulate the other fields (it's important to not confuse the
caller by doing so), so that's not viable for sha1.
On Tue, Dec 13, 2022 at 06:43:30PM +0800, Herbert Xu wrote:
> -asmlinkage void sha1_block_data_order(u32 *digest,
> - const unsigned char *data, unsigned int rounds);
> +asmlinkage void sha1_block_data_order(struct sha1_state *digest,
> + const u8 *data, int rounds);
The last parameter should be called 'blocks', not 'rounds'.
> int sha1_update_arm(struct shash_desc *desc, const u8 *data,
> unsigned int len)
> {
> - /* make sure casting to sha1_block_fn() is safe */
> + /* make sure signature matches sha1_block_fn() */
> BUILD_BUG_ON(offsetof(struct sha1_state, state) != 0);
The above comment doesn't really make sense, since making sure function
signatures match is the responsibility of the compiler.
A better comment would be:
/* sha1_block_data_order() expects the actual state at the beginning. */
It would also be helpful to add a comment to the definition of struct
sha1_state, analogous to the comment in struct blake2s_state:
struct sha1_state {
/* 'state' is used by assembly code, so keep it as-is. */
u32 state[SHA1_DIGEST_SIZE / 4];
- Eric