LinuxLists.cc - [PATCH] crypto: x86/aes-ni - add missing error checks in XTS code

2021-07-16 16:56:56

Subject: [PATCH] crypto: x86/aes-ni - add missing error checks in XTS code

The updated XTS code fails to check the return code of skcipher_walk_virt,
which may lead to skcipher_walk_abort() or skcipher_walk_done() being called
while the walk argument is in an inconsistent state.

So check the return value after each such call, and bail on errors.

Fixes: 2481104fe98d ("crypto: x86/aes-ni-xts - rewrite and drop indirections via glue helper")
Reported-by: Dave Hansen <[email protected]>
Reported-by: syzbot <[email protected]>
Signed-off-by: Ard Biesheuvel <[email protected]>
---
arch/x86/crypto/aesni-intel_glue.c | 5 +++++
1 file changed, 5 insertions(+)

diff --git a/arch/x86/crypto/aesni-intel_glue.c b/arch/x86/crypto/aesni-intel_glue.c
index 2144e54a6c89..388643ca2177 100644
--- a/arch/x86/crypto/aesni-intel_glue.c
+++ b/arch/x86/crypto/aesni-intel_glue.c
@@ -849,6 +849,8 @@ static int xts_crypt(struct skcipher_request *req, bool encrypt)
return -EINVAL;

err = skcipher_walk_virt(&walk, req, false);
+ if (err)
+ return err;

if (unlikely(tail > 0 && walk.nbytes < walk.total)) {
int blocks = DIV_ROUND_UP(req->cryptlen, AES_BLOCK_SIZE) - 2;
@@ -862,7 +864,10 @@ static int xts_crypt(struct skcipher_request *req, bool encrypt)
skcipher_request_set_crypt(&subreq, req->src, req->dst,
blocks * AES_BLOCK_SIZE, req->iv);
req = &subreq;
+
err = skcipher_walk_virt(&walk, req, false);
+ if (err)
+ return err;
} else {
tail = 0;
}
--
2.30.2

2021-07-16 23:58:59

by Eric Biggers

[permalink] [raw]

Subject: Re: [PATCH] crypto: x86/aes-ni - add missing error checks in XTS code

On Fri, Jul 16, 2021 at 06:54:03PM +0200, Ard Biesheuvel wrote:
> The updated XTS code fails to check the return code of skcipher_walk_virt,
> which may lead to skcipher_walk_abort() or skcipher_walk_done() being called
> while the walk argument is in an inconsistent state.
>
> So check the return value after each such call, and bail on errors.
>
> Fixes: 2481104fe98d ("crypto: x86/aes-ni-xts - rewrite and drop indirections via glue helper")

Add Cc stable?

> Reported-by: Dave Hansen <[email protected]>
> Reported-by: syzbot <[email protected]>
> Signed-off-by: Ard Biesheuvel <[email protected]>
> ---
> arch/x86/crypto/aesni-intel_glue.c | 5 +++++
> 1 file changed, 5 insertions(+)

Reviewed-by: Eric Biggers <[email protected]>

- Eric

2021-07-19 07:12:43

by Ard Biesheuvel

[permalink] [raw]

Subject: Re: [PATCH] crypto: x86/aes-ni - add missing error checks in XTS code

On Sat, 17 Jul 2021 at 01:58, Eric Biggers <[email protected]> wrote:
>
> On Fri, Jul 16, 2021 at 06:54:03PM +0200, Ard Biesheuvel wrote:
> > The updated XTS code fails to check the return code of skcipher_walk_virt,
> > which may lead to skcipher_walk_abort() or skcipher_walk_done() being called
> > while the walk argument is in an inconsistent state.
> >
> > So check the return value after each such call, and bail on errors.
> >
> > Fixes: 2481104fe98d ("crypto: x86/aes-ni-xts - rewrite and drop indirections via glue helper")
>
> Add Cc stable?
>
> > Reported-by: Dave Hansen <[email protected]>
> > Reported-by: syzbot <[email protected]>
> > Signed-off-by: Ard Biesheuvel <[email protected]>
> > ---
> > arch/x86/crypto/aesni-intel_glue.c | 5 +++++
> > 1 file changed, 5 insertions(+)
>
> Reviewed-by: Eric Biggers <[email protected]>
>

Thanks Eric. Herbert can add the cc:stable if he decides to, but IIRC,
he prefers relying on the fixes: tag only.

2021-07-23 06:55:07

by Herbert Xu

[permalink] [raw]

Subject: Re: [PATCH] crypto: x86/aes-ni - add missing error checks in XTS code

On Fri, Jul 16, 2021 at 06:54:03PM +0200, Ard Biesheuvel wrote:
> The updated XTS code fails to check the return code of skcipher_walk_virt,
> which may lead to skcipher_walk_abort() or skcipher_walk_done() being called
> while the walk argument is in an inconsistent state.
>
> So check the return value after each such call, and bail on errors.
>
> Fixes: 2481104fe98d ("crypto: x86/aes-ni-xts - rewrite and drop indirections via glue helper")
> Reported-by: Dave Hansen <[email protected]>
> Reported-by: syzbot <[email protected]>
> Signed-off-by: Ard Biesheuvel <[email protected]>
> ---
> arch/x86/crypto/aesni-intel_glue.c | 5 +++++
> 1 file changed, 5 insertions(+)

Patch applied. Thanks.
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

2021-08-03 02:02:31

by kernel test robot

[permalink] [raw]

Subject: [crypto] ed610fb786: will-it-scale.per_thread_ops 4.6% improvement

Greeting,

FYI, we noticed a

commit: ed610fb7867eff838da url: in testcase: will-it-scale
on test machine: 88 with following parameters:

nr_task: 100%
mode: thread
test: getppid1
cpufreq_governor: performance
ucode: 0x5003006

test-description: test-url: Thanks,
Oliver Sang

4.6% improvement of will-it-scale.per_thread_ops due to commit:
286444016ae58fa4f86b2 ("[PATCH] crypto: x86/aes-ni - add missing error checks in XTS code")
ub.com/0day-ci/linux/commits/Ard-Biesheuvel/crypto-x86-aes-ni-add-missing-error-checks-in-XTS-code/20210718-100125">https://github.com/0day-ci/linux/commits/Ard-Biesheuvel/crypto-x86-aes-ni-add-missing-error-checks-in-XTS-code/20210718-100125
.kernel.org/cgit/linux/kernel/git/herbert/cryptodev-2.6.git">https://git.kernel.org/cgit/linux/kernel/git/herbert/cryptodev-2.6.git master
threads 2 sockets Intel(R) Xeon(R) Gold 6238M CPU @ 2.10GHz with 128G memory
Will It Scale takes a testcase and runs it from 1 through to n parallel copies to see if the testcase will scale. It builds both a process and threads based test in order to see any differences between the two.
/github.com/antonblanchard/will-it-scale">https://github.com/antonblanchard/will-it-scale
----------------------------------------------------------------------->
href="https://github.com/intel/lkp-tests.git">https://github.com/intel/lkp-tests.git
job.yaml # job file is attached in this email
--compatible job.yaml # generate the yaml file for lkp run
generated-yaml-file
==============================================================
config/mode/nr_task/rootfs/tbox_group/test/testcase/ucode:
rhel-8.3/thread/100%/debian-10.4-x86_64-20200603.cgz/lkp-csl-2sp9/getppid1/will-it-scale/0x5003006
hisilicon/sec - fix the process of disabling sva prefetching")
x86/aes-ni - add missing error checks in XTS code")
7eff838da28644401
-----------------
%stddev
\
+4.6% 7.601e+08 will-it-scale.88.threads
+4.6% 8637651 will-it-scale.per_thread_ops
+4.6% 7.601e+08 will-it-scale.workload
1818 ? 16% interrupts.CPU33.CAL:Function_call_interrupts
336.29 ? 9% interrupts.CPU37.RES:Rescheduling_interrupts
349.43 ? 20% interrupts.CPU63.RES:Rescheduling_interrupts
5546 ? 34% interrupts.CPU75.NMI:Non-maskable_interrupts
5546 ? 34% interrupts.CPU75.PMI:Performance_monitoring_interrupts
365.29 ? 27% interrupts.CPU77.RES:Rescheduling_interrupts
1903 ? 42% perf-sched.wait_and_delay.avg.ms.do_syslog.part.0.kmsg_read.vfs_read
5706 ? 42% perf-sched.wait_and_delay.max.ms.do_syslog.part.0.kmsg_read.vfs_read
1080 ?237% perf-sched.wait_and_delay.max.ms.exit_to_user_mode_prepare.syscall_exit_to_user_mode.do_syscall_64.entry_SYSCALL_64_after_hwframe
5139 ? 10% perf-sched.wait_and_delay.max.ms.smpboot_thread_fn.kthread.ret_from_fork
1903 ? 42% perf-sched.wait_time.avg.ms.do_syslog.part.0.kmsg_read.vfs_read
0.13 ? 19% perf-sched.wait_time.avg.ms.exit_to_user_mode_prepare.irqentry_exit_to_user_mode.asm_sysvec_apic_timer_interrupt.[unknown]
5705 ? 42% perf-sched.wait_time.max.ms.do_syslog.part.0.kmsg_read.vfs_read
1080 ?237% perf-sched.wait_time.max.ms.exit_to_user_mode_prepare.syscall_exit_to_user_mode.do_syscall_64.entry_SYSCALL_64_after_hwframe
5139 ? 10% perf-sched.wait_time.max.ms.smpboot_thread_fn.kthread.ret_from_fork
+4.6% 3.954e+10 perf-stat.i.branch-instructions
0.47 perf-stat.i.branch-miss-rate%
479520 ? 4% perf-stat.i.cache-misses
1.31 perf-stat.i.cpi
652145 ? 6% perf-stat.i.cycles-between-cache-misses
+4.6% 6.299e+10 perf-stat.i.dTLB-loads
0.00 perf-stat.i.dTLB-store-miss-rate%
-1.6% 146697 perf-stat.i.dTLB-store-misses
+4.5% 4.401e+10 perf-stat.i.dTLB-stores
-4.9% 2.238e+08 perf-stat.i.iTLB-load-misses
+4.6% 1.841e+11 perf-stat.i.instructions
+9.5% 826.97 perf-stat.i.instructions-per-iTLB-miss
0.76 perf-stat.i.ipc
1665 perf-stat.i.metric.M/sec
0.47 perf-stat.overall.branch-miss-rate%
1.32 perf-stat.overall.cpi
-9.0% 500080 ? 4% perf-stat.overall.cycles-between-cache-misses
0.00 perf-stat.overall.dTLB-store-miss-rate%
822.71 perf-stat.overall.instructions-per-iTLB-miss
0.76 perf-stat.overall.ipc
+4.6% 3.941e+10 perf-stat.ps.branch-instructions
+9.9% 483612 ? 4% perf-stat.ps.cache-misses
+4.6% 6.278e+10 perf-stat.ps.dTLB-loads
-1.6% 146461 perf-stat.ps.dTLB-store-misses
+4.5% 4.386e+10 perf-stat.ps.dTLB-stores
-4.9% 2.231e+08 perf-stat.ps.iTLB-load-misses
+4.6% 1.835e+11 perf-stat.ps.instructions
+4.6% 5.534e+13 perf-stat.total.instructions
10.51 ? 3% perf-profile.calltrace.cycles-pp.syscall_exit_to_user_mode.do_syscall_64.entry_SYSCALL_64_after_hwframe.getppid
30.28 perf-profile.calltrace.cycles-pp.do_syscall_64.entry_SYSCALL_64_after_hwframe.getppid
38.22 perf-profile.calltrace.cycles-pp.entry_SYSCALL_64_after_hwframe.getppid
7.55 ? 4% perf-profile.calltrace.cycles-pp.exit_to_user_mode_prepare.syscall_exit_to_user_mode.do_syscall_64.entry_SYSCALL_64_after_hwframe.getppid
1.50 ? 3% perf-profile.calltrace.cycles-pp.syscall_exit_to_user_mode_prepare.syscall_exit_to_user_mode.do_syscall_64.entry_SYSCALL_64_after_hwframe.getppid
94.26 perf-profile.calltrace.cycles-pp.getppid
3.12 perf-profile.calltrace.cycles-pp.entry_SYSCALL_64_safe_stack.getppid
3.20 perf-profile.calltrace.cycles-pp.syscall_enter_from_user_mode.do_syscall_64.entry_SYSCALL_64_after_hwframe.getppid
9.37 perf-profile.calltrace.cycles-pp.testcase
44.56 perf-profile.calltrace.cycles-pp.__entry_text_start.getppid
11.64 ? 3% perf-profile.children.cycles-pp.syscall_exit_to_user_mode
31.19 perf-profile.children.cycles-pp.do_syscall_64
38.96 perf-profile.children.cycles-pp.entry_SYSCALL_64_after_hwframe
7.92 ? 4% perf-profile.children.cycles-pp.exit_to_user_mode_prepare
1.81 ? 2% perf-profile.children.cycles-pp.syscall_exit_to_user_mode_prepare
1.07 perf-profile.children.cycles-pp.rcu_read_unlock_strict
3.34 perf-profile.children.cycles-pp.entry_SYSCALL_64_safe_stack
3.28 perf-profile.children.cycles-pp.syscall_enter_from_user_mode
5.52 perf-profile.children.cycles-pp.testcase
23.10 perf-profile.children.cycles-pp.syscall_return_via_sysret
28.77 perf-profile.children.cycles-pp.__entry_text_start
6.81 ? 4% perf-profile.self.cycles-pp.exit_to_user_mode_prepare
1.43 ? 2% perf-profile.self.cycles-pp.syscall_exit_to_user_mode_prepare
3.69 perf-profile.self.cycles-pp.__x64_sys_getppid
0.73 perf-profile.self.cycles-pp.rcu_read_unlock_strict
2.31 perf-profile.self.cycles-pp.syscall_exit_to_user_mode
0.72 ? 3% perf-profile.self.cycles-pp.rcu_nocb_flush_deferred_wakeup
7.90 perf-profile.self.cycles-pp.entry_SYSCALL_64_after_hwframe
3.31 perf-profile.self.cycles-pp.entry_SYSCALL_64_safe_stack
2.55 perf-profile.self.cycles-pp.syscall_enter_from_user_mode
12.96 perf-profile.self.cycles-pp.__entry_text_start
22.77 perf-profile.self.cycles-pp.syscall_return_via_sysret
20.14 perf-profile.self.cycles-pp.getppid

will-it-scale.per_thread_ops

-------------------------------------------------+
|
|
O O |
OO O OO OOOO OOO |
|
|
|
+.++++.+ |
.++ ++. : : |
: |
: |
: + .+ + +. + |
+ .+ :+ + +.++ ++++.++++.+ ++.+|
-------------------------------------------------+

estimated based on internal Intel analysis and are provided
purposes only. Any difference in system hardware or software
may affect actual performance.
Open Source Technology Center
org/hyperkitty/list/lkp@lists.01.org">https://lists.01.org/hyperkitty/list/[email protected] Intel Corporation

Attachments:

(No filename) (11.78 kB)
config-5.13.0-rc1-00150-ged610fb7867e (176.63 kB)
job-script (8.10 kB)
job.yaml (5.37 kB)
reproduce (349.00 B)
Download all attachments