Messages in this thread | | | Date | Wed, 15 Nov 2023 10:05:43 +0800 | Subject | Re: [PATCH] crypto: x86/sm2 -add Zhaoxin SM2 algorithm implementation | From | LeoLiu-oc <> |
| |
在 2023/11/9 23:00, Dave Hansen 写道: > On 11/9/23 01:47, LeoLiu-oc wrote: > ... >> diff --git a/arch/x86/crypto/Kconfig b/arch/x86/crypto/Kconfig >> index 9bbfd01cfa2f..a771a9da2abd 100644 >> --- a/arch/x86/crypto/Kconfig >> +++ b/arch/x86/crypto/Kconfig >> @@ -519,4 +519,15 @@ config CRYPTO_CRCT10DIF_PCLMUL >> Architecture: x86_64 using: >> - PCLMULQDQ (carry-less multiplication) >> >> +config CRYPTO_SM2_ZHAOXIN_GMI >> + tristate "SM2 Cipher algorithm (Zhaoxin GMI Instruction)" >> + depends on X86 > > Why does this not have "depends on CPU_SUP_ZHAOXIN"? > Thank you for your suggestion. It is indeed necessary to add a dependency on CPU_SUP_ZHAOXIN here, which will be updated in the next version.
> Also, the assembly in here looks suspiciously like x86_64 assembly. > How, exactly, does this build when !X86_64? > The assembly code will be modified to be embedded in the .c file in the next version, which will be compatible with !X86_64.
>> + select CRYPTO_AKCIPHER >> + select CRYPTO_MANAGER >> + help >> + SM2 (ShangMi 2) public key algorithm by Zhaoxin GMI Instruction >> + >> + Published by State Encryption Management Bureau, China, >> + as specified by OSCCA GM/T 0003.1-2012 -- 0003.5-2012. >> + >> endmenu >> diff --git a/arch/x86/crypto/Makefile b/arch/x86/crypto/Makefile >> index 9aa46093c91b..c23b328a3ecd 100644 >> --- a/arch/x86/crypto/Makefile >> +++ b/arch/x86/crypto/Makefile >> @@ -109,6 +109,9 @@ aria-aesni-avx2-x86_64-y := aria-aesni-avx2-asm_64.o aria_aesni_avx2_glue.o >> obj-$(CONFIG_CRYPTO_ARIA_GFNI_AVX512_X86_64) += aria-gfni-avx512-x86_64.o >> aria-gfni-avx512-x86_64-y := aria-gfni-avx512-asm_64.o aria_gfni_avx512_glue.o >> >> +obj-$(CONFIG_CRYPTO_SM2_ZHAOXIN_GMI) += sm2-zhaoxin-gmi.o >> +sm2-zhaoxin-gmi-y := sm2-zhaoxin-gmi_asm.o sm2-zhaoxin-gmi_glue.o >> + >> quiet_cmd_perlasm = PERLASM $@ >> cmd_perlasm = $(PERL) $< > $@ >> $(obj)/%.S: $(src)/%.pl FORCE >> diff --git a/arch/x86/crypto/sm2-zhaoxin-gmi_asm.S b/arch/x86/crypto/sm2-zhaoxin-gmi_asm.S >> new file mode 100644 >> index 000000000000..4ee5194557a0 >> --- /dev/null >> +++ b/arch/x86/crypto/sm2-zhaoxin-gmi_asm.S >> @@ -0,0 +1,59 @@ >> +/* SPDX-License-Identifier: GPL-2.0-or-later */ >> +/* >> + * Zhaoxin GMI implementation of a SM2 function >> + * >> + * Copyright(c) 2023 Zhaoxin Semiconductor LTD. >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of version 2 of the GNU General Public License as >> + * published by the Free Software Foundation. >> + * >> + * This program is distributed in the hope that it will be useful, but >> + * WITHOUT ANY WARRANTY; without even the implied warranty of >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU >> + * General Public License for more details. >> + * >> + * Contact Information: >> + * YunShen <YunShen@zhaoxin.com> >> + */ > > I thought we were using SPDX instead of blown-up license text now. Why > does this need the GPLv2 blurb? > Thank you for your reminder. I will remove it in the subsequent version.
>> +#include <linux/linkage.h> >> + >> +#define KEY_PTR %rdi /* 1st arg */ >> +#define HASH_PTR %rsi /* 2nd arg */ >> +#define SIG_PTR %rdx /* 3rd arg */ >> +#define SCRATCH_PTR %rcx /* 4rd arg */ >> +#define VER_RESULT %rax /* ret */ >> + >> +.text >> +.align 32 >> +############################################################################### >> +# int zhaoxin_gmi_sm2_verify ( >> +# unsigned char *key, /*key*/ >> +# unsigned char *hash, /*hash*/ >> +# unsigned char *sig, /*signature*/ >> +# unsigned char *scratch /*8 kbytes scratch space*/ >> +# ); >> +############################################################################### > > This comment is just a waste of space. The types here are completely > uninformative and the comments: > > unsigned char *key, /*key*/ > > are, um, equally uninformative considering the #defines above. > The assembly code here will be abandoned in the next version and will be modified to be embedded in the C language code. There will be no such problem in the future.
>> +SYM_FUNC_START(zhaoxin_gmi_sm2_verify) >> + push %r12 >> + push %rbx > > Please mention that these are callee-saved registers that the > instruction will clobber. > As mentioned above.
>> + mov HASH_PTR, %rax >> + mov KEY_PTR, %rbx >> + mov SIG_PTR, %r12 >> + mov $8, %rdx >> + mov SCRATCH_PTR, %rsi >> + mov %r12, %rdi >> + >> + .byte 0XF2 >> + .byte 0X0F >> + .byte 0XA6 >> + .byte 0XC0 > > Please look around the codebase and see what folks do with assembly for > instructions that might not be supported in the assembler. There's an > existing standard here. > As mentioned above.
>> + mov %rcx, %rax >> + >> + pop %rbx >> + pop %r12 >> + >> + RET >> +SYM_FUNC_END(zhaoxin_gmi_sm2_verify) >> diff --git a/arch/x86/crypto/sm2-zhaoxin-gmi_glue.c b/arch/x86/crypto/sm2-zhaoxin-gmi_glue.c >> new file mode 100644 >> index 000000000000..4d0d18f68266 >> --- /dev/null >> +++ b/arch/x86/crypto/sm2-zhaoxin-gmi_glue.c >> @@ -0,0 +1,145 @@ >> +// SPDX-License-Identifier: GPL-2.0-or-later >> +/* >> + * SM2 asymmetric public-key algorithm >> + * as specified by OSCCA GM/T 0003.1-2012 -- 0003.5-2012 SM2 and >> + * described at https://tools.ietf.org/html/draft-shen-sm2-ecdsa-02 >> + * >> + * Copyright (c) 2023 Shanghai Zhaoxin Semiconductor LTD. >> + * Authors: YunShen <yunshen@zhaoxin.com> >> + */ >> + >> +#include <linux/module.h> >> +#include <linux/mpi.h> >> +#include <crypto/internal/akcipher.h> >> +#include <crypto/akcipher.h> >> +#include <crypto/sm2.h> >> +#include <asm/cpufeature.h> >> +#include <asm/processor.h> >> +#include <asm/cpu_device_id.h> >> + >> +#define SCRATCH_SIZE (4 * 2048) >> + >> +asmlinkage int zhaoxin_gmi_sm2_verify(unsigned char *key, unsigned char *hash, unsigned char *sig, >> + unsigned char *scratch); >> + >> +struct sm2_cipher_data { >> + u8 pub_key[65]; /* public key */ >> +}; >> + >> +/* Load supported features of the CPU to see if the SM2 is available. */ >> +static int zhaoxin_gmi_available(void) >> +{ >> + if (!boot_cpu_has(X86_FEATURE_SM2_EN)) { >> + pr_err("can't enable hardware SM2 if Zhaoxin GMI SM2 is not enabled\n"); >> + return -ENODEV; >> + } >> + return 0; >> +} > > How can this code even be reached if X86_FEATURE_SM2_EN is clear? How > will the cipher even get registered? > SM2 has two macro definitions, one representing the existence of SM2 functionality; the other SM2_EN represents that the functionality is enabled and can be modified in the firmware. In the initialization function, the detection of X86_FEATURE_SM2 will be performed to verify if SM2 functionality is supported. This is the detection of X86_FEATURE_SM2_EN to verify whether the SM2 function is enabled in the firmware.
>> +/* Zhaoxin sm2 verify function */ >> +static int _zhaoxin_sm2_verify(struct sm2_cipher_data *ec, unsigned char *hash, unsigned char *sig) >> +{ >> + int ret = -EKEYREJECTED; >> + uint64_t f_ok = 0; > > This is an odd type choice since this is far from a user/kernel ABI and > also the zhaoxin_gmi_sm2_verify() declared return type is 'int'. > Yes, it will be modified in the next version.
>> + unsigned char *scratch = kzalloc(SCRATCH_SIZE, GFP_KERNEL); >> + >> + f_ok = zhaoxin_gmi_sm2_verify(ec->pub_key, hash, sig, scratch); >> + if (f_ok == 1) >> + ret = 0; >> + >> + kfree(scratch); >> + >> + return ret; >> +} >> + >> +static int zhaoxin_sm2_verify(struct akcipher_request *req) >> +{ >> + struct crypto_akcipher *tfm = crypto_akcipher_reqtfm(req); >> + struct sm2_cipher_data *ec = akcipher_tfm_ctx(tfm); >> + unsigned char *buffer; >> + int ret; >> + >> + buffer = kmalloc(req->src_len + req->dst_len, GFP_KERNEL); >> + if (!buffer) >> + return -ENOMEM; >> + >> + sg_pcopy_to_buffer(req->src, sg_nents_for_len(req->src, req->src_len + req->dst_len), >> + buffer, req->src_len + req->dst_len, 0); > > While sg_pcopy_to_buffer() calls in general seem > readablility-challenged, this one in particular is pretty bad. Please > break this up logically (one line per argument seems commonplace) or use > some helper intermediate variables. I do notice that 'req->src_len + > req->dst_len' is repeated quite a few times in this function, for instance. > Yes, this change will make the code clearer, and will be modified in the next version as you suggested.
>> + ret = _zhaoxin_sm2_verify(ec, buffer + req->src_len, buffer); >> + kfree(buffer); >> + >> + return ret; >> +} >> + >> +static int zhaoxin_sm2_set_pub_key(struct crypto_akcipher *tfm, const void *key, >> + unsigned int keylen) >> +{ >> + struct sm2_cipher_data *ec = akcipher_tfm_ctx(tfm); >> + >> + memcpy(ec->pub_key, key, keylen); >> + >> + return 0; >> +} >> + >> +static unsigned int zhaoxin_sm2_max_size(struct crypto_akcipher *tfm) >> +{ >> + /* Unlimited max size */ >> + return PAGE_SIZE; >> +} > > I don't know the crypto API well, but does this mean that for every 4k > of data that this code deals with, it needs to kmalloc()/kfree() 8k? > The return value of the zhaoxin_sm2_max_size function is the maximum length of the algorithm's output buffer.Generally,the length of the output data of the SM2 algorithm is not greater than PAGE_SIZE. In addition, it also references the setting of the software-implemented SM2 algorithm in the kernel.
>> +static int zhaoxin_sm2_init_tfm(struct crypto_akcipher *tfm) >> +{ >> + return zhaoxin_gmi_available(); >> +} >> + >> +static void zhaoxin_sm2_exit_tfm(struct crypto_akcipher *tfm) >> +{ >> + struct sm2_cipher_data *ec = akcipher_tfm_ctx(tfm); >> + >> + memset(ec, 0, sizeof(*ec)); >> +} >> + >> +static struct akcipher_alg zhaoxin_sm2 = { >> + .verify = zhaoxin_sm2_verify, >> + .set_pub_key = zhaoxin_sm2_set_pub_key, >> + .max_size = zhaoxin_sm2_max_size, >> + .init = zhaoxin_sm2_init_tfm, >> + .exit = zhaoxin_sm2_exit_tfm, >> + .base = { >> + .cra_name = "sm2", >> + .cra_driver_name = "zhaoxin-gmi-sm2", >> + .cra_priority = 150, >> + .cra_module = THIS_MODULE, >> + .cra_ctxsize = sizeof(struct sm2_cipher_data), >> + }, >> +}; >> + >> +static const struct x86_cpu_id zhaoxin_sm2_cpu_ids[] = { >> + X86_MATCH_FEATURE(X86_FEATURE_SM2, NULL), >> + {} >> +}; >> +MODULE_DEVICE_TABLE(x86cpu, zhaoxin_sm2_cpu_ids); >> + >> +static int __init zhaoxin_sm2_init(void) >> +{ >> + if (!x86_match_cpu(zhaoxin_sm2_cpu_ids)) { >> + pr_err("The CPU isn't support hardware SM2.\n"); >> + return -ENODEV; >> + } >> + >> + return crypto_register_akcipher(&zhaoxin_sm2); >> +} > > Again, please look around the code base. There are a bunch of crypto > drivers that do checks like this. *None* have a pr_err(). Few spew any > spam to dmesg. Why does this one? > OK, The pr_err statement here will be removed in the next version.
>> +static void __exit zhaoxin_sm2_exit(void) >> +{ >> + crypto_unregister_akcipher(&zhaoxin_sm2); >> +} >> + >> +module_init(zhaoxin_sm2_init); >> +module_exit(zhaoxin_sm2_exit); >> + >> +MODULE_LICENSE("GPL"); >> +MODULE_AUTHOR("YunShen <yunshen@zhaoxin.com>"); >> +MODULE_DESCRIPTION("SM2 Zhaoxin GMI Algorithm"); >> +MODULE_ALIAS_CRYPTO("zhaoxin-gmi-sm2"); >> diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h >> index 4af140cf5719..07a78ec83bed 100644 >> --- a/arch/x86/include/asm/cpufeatures.h >> +++ b/arch/x86/include/asm/cpufeatures.h >> @@ -146,6 +146,8 @@ >> #define X86_FEATURE_HYPERVISOR ( 4*32+31) /* Running on a hypervisor */ >> >> /* VIA/Cyrix/Centaur-defined CPU features, CPUID level 0xC0000001, word 5 */ >> +#define X86_FEATURE_SM2 (5*32 + 0) /* SM2 ZhaoXin GMI present */ >> +#define X86_FEATURE_SM2_EN (5*32 + 1) /* SM2 ZhaoXin GMI enabled */ > > Ahh, so I actually misread the two CPUID flags that got checked earlier. > > What does it take to get "GMI enabled"? How can it be present but not > enabled? > As mentioned earlier, SM2 and SM2_EN represent the existence and enablement of SM2 functionality, respectively. SM2_EN can be turned on or off in the firmware.
Best Regards LeoLiu-oc
| |