Re: [PATCH] crypto: DRBG - guard uninstantion by lock

[Dmitry Vyukov]

On Wed, Apr 11, 2018 at 4:26 PM, Stephan Müller  wrote:
> Hi Dimitry,
>
> This fix prevents the kernel from crashing when injecting the fault.

Good!

> Stack traces are yet shown but I guess that is expected every time
> a fault is injected.

Yes, nothing to fix here.

> As to why KASAN did not notice this one, I am not sure. Maybe it is
> because I use two buffer pointers to point to (almost) the same memory
> (one that is aligned and one pointing to the complete buffer)?

After looking at the fix, I figured out what happened with KASAN.
Filed https://bugzilla.kernel.org/show_bug.cgi?id=199359. In short,
tricky interplay between kzfree, ksize and double-free detection.
If KASAN worked as intended it would give a nice "double-free in this
stack for object allocated in this stack and previously freed in this
stack", which would probably make debugging much simpler.


> ---8<---
>
> During freeing of the internal buffers used by the DRBG, set the pointer
> to NULL. It is possible that the context with the freed buffers is
> reused. In case of an error during initialization where the pointers
> do not yet point to allocated memory, the NULL value prevents a double
> free.
>
> Signed-off-by: Stephan Mueller 
> Reported-by: syzbot+75397ee3df5c70164...@syzkaller.appspotmail.com
> ---
>  crypto/drbg.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/crypto/drbg.c b/crypto/drbg.c
> index 4faa2781c964..466a112a4446 100644
> --- a/crypto/drbg.c
> +++ b/crypto/drbg.c
> @@ -1134,8 +1134,10 @@ static inline void drbg_dealloc_state(struct 
> drbg_state *drbg)
> if (!drbg)
> return;
> kzfree(drbg->Vbuf);
> +   drbg->Vbuf = NULL;
> drbg->V = NULL;
> kzfree(drbg->Cbuf);
> +   drbg->Cbuf = NULL;
> drbg->C = NULL;
> kzfree(drbg->scratchpadbuf);
> drbg->scratchpadbuf = NULL;
> --
> 2.14.3
>
>
>
>
> --
> You received this message because you are subscribed to the Google Groups 
> "syzkaller-bugs" group.
> To unsubscribe from this group and stop receiving emails from it, send an 
> email to syzkaller-bugs+unsubscr...@googlegroups.com.
> To view this discussion on the web visit 
> https://groups.google.com/d/msgid/syzkaller-bugs/2186798.qrgUIDAn9S%40positron.chronox.de.
> For more options, visit https://groups.google.com/d/optout.

Re: [PATCH] crypto: DRBG - guard uninstantion by lock

[Stephan Müller]

Hi Dimitry,

This fix prevents the kernel from crashing when injecting the fault. 
Stack traces are yet shown but I guess that is expected every time
a fault is injected.

As to why KASAN did not notice this one, I am not sure. Maybe it is
because I use two buffer pointers to point to (almost) the same memory
(one that is aligned and one pointing to the complete buffer)?

---8<---

During freeing of the internal buffers used by the DRBG, set the pointer
to NULL. It is possible that the context with the freed buffers is
reused. In case of an error during initialization where the pointers
do not yet point to allocated memory, the NULL value prevents a double
free.

Signed-off-by: Stephan Mueller 
Reported-by: syzbot+75397ee3df5c70164...@syzkaller.appspotmail.com
---
 crypto/drbg.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/crypto/drbg.c b/crypto/drbg.c
index 4faa2781c964..466a112a4446 100644
--- a/crypto/drbg.c
+++ b/crypto/drbg.c
@@ -1134,8 +1134,10 @@ static inline void drbg_dealloc_state(struct drbg_state 
*drbg)
if (!drbg)
return;
kzfree(drbg->Vbuf);
+   drbg->Vbuf = NULL;
drbg->V = NULL;
kzfree(drbg->Cbuf);
+   drbg->Cbuf = NULL;
drbg->C = NULL;
kzfree(drbg->scratchpadbuf);
drbg->scratchpadbuf = NULL;
-- 
2.14.3

Re: [PATCH] crypto: DRBG - guard uninstantion by lock

[Stephan Mueller]

Am Mittwoch, 11. April 2018, 14:29:45 CEST schrieb Dmitry Vyukov:

Hi Dmitry,
> 
> What do you mean by description of the fault?
> It's kernel standard FAULT_INJECTION facility, it injects faults
> mainly into kmalloc/slab_alloc (also in a bunch of other things, but
> in this case this seems to be kmalloc). In the repro you can see that
> it's injecting a fault into 8-th malloc in the setsockopt syscall.

I am now able to reproduce it.

I think I have a smoking gun, but let me test it first.

Ciao
Stephan

Re: [PATCH] crypto: DRBG - guard uninstantion by lock

[Dmitry Vyukov]

On Tue, Apr 10, 2018 at 5:35 PM, Stephan Mueller  wrote:
> Am Dienstag, 10. April 2018, 17:23:46 CEST schrieb Dmitry Vyukov:
>
> Hi Dmitry,
>
>> Stephan,
>>
>> Do you have any hypothesis as to why this is not detected by KASAN and
>> causes silent corruptions?
>> We generally try to understand such cases and improve KASAN so that it
>> catches such cases more reliably and they do not cause splashes of
>> random crashes on syzbot.
>
> I do not have any hypothesis at this point. I know that you induce some fault.
> As you mentioned the drbg_kcapi_seed function, I was looking through the error
> code paths to see whether some error handlers trip over each other. But all is
> guesswork so far. And I am not even sure whether the bug is in the DRBG code
> base.
>
> Looking into the trace you sent, I see a NULL pointer dereference. At one
> point there is also the drbg_init_hash_kernel that is called. But nowhere I
> see any smoking gun.
>
> Could you please give me a description of the fault you are inducing?

Hi Stephan,

What do you mean by description of the fault?
It's kernel standard FAULT_INJECTION facility, it injects faults
mainly into kmalloc/slab_alloc (also in a bunch of other things, but
in this case this seems to be kmalloc). In the repro you can see that
it's injecting a fault into 8-th malloc in the setsockopt syscall.

I wonder why you can't reproduce it. I can trigger it reliably in a
qemu. Let's try this:
I have upstream kernel on b284d4d5a6785f8cd07eda2646a95782373cd01e.
Here is my config:
https://gist.githubusercontent.com/dvyukov/f843ea09bc5b9439a820c8e809a5501d/raw/ad330e9b6b710f57f63b61590747b48230e5cb61/gistfile1.txt
Here is the compiler:
https://storage.googleapis.com/syzkaller/gcc-8.0.1-20180301.tar.gz
Build as: make -jN CC=that/gcc/bin/gcc
Then I start qemu as:

qemu-system-x86_64 -hda wheezy.img -net
user,host=10.0.2.10,hostfwd=tcp::10022-:22 -net nic -nographic -kernel
arch/x86/boot/bzImage -append "kvm-intel.nested=1
kvm-intel.unrestricted_guest=1 kvm-intel.ept=1
kvm-intel.flexpriority=1 kvm-intel.vpid=1
kvm-intel.emulate_invalid_guest_state=1 kvm-intel.eptad=1
kvm-intel.enable_shadow_vmcs=1 kvm-intel.pml=1
kvm-intel.enable_apicv=1 console=ttyS0 root=/dev/sda
earlyprintk=serial slub_debug=UZ vsyscall=native rodata=n oops=panic
panic_on_warn=1 panic=86400" -enable-kvm -pidfile vm_pid -m 2G -smp 4
-cpu host

You can find the wheezy.img and ssh key for it here:
https://github.com/google/syzkaller/blob/master/docs/syzbot.md#crash-does-not-reproduce

Then I compile this program:
https://gist.githubusercontent.com/dvyukov/1dd75d55efd238e7207af1cc38478b3a/raw/403859b56b161a6fbb158e8953fac5bb6e73b1a1/gistfile1.txt
as: gcc prog.c -static -m32

Run in the qemu and within a minute it gives me the crash.

Re: [PATCH] crypto: DRBG - guard uninstantion by lock

[Stephan Mueller]

Am Dienstag, 10. April 2018, 17:23:46 CEST schrieb Dmitry Vyukov:

Hi Dmitry,

> Stephan,
> 
> Do you have any hypothesis as to why this is not detected by KASAN and
> causes silent corruptions?
> We generally try to understand such cases and improve KASAN so that it
> catches such cases more reliably and they do not cause splashes of
> random crashes on syzbot.

I do not have any hypothesis at this point. I know that you induce some fault. 
As you mentioned the drbg_kcapi_seed function, I was looking through the error 
code paths to see whether some error handlers trip over each other. But all is 
guesswork so far. And I am not even sure whether the bug is in the DRBG code 
base.

Looking into the trace you sent, I see a NULL pointer dereference. At one 
point there is also the drbg_init_hash_kernel that is called. But nowhere I 
see any smoking gun.

Could you please give me a description of the fault you are inducing?

Ciao
Stephan

Re: [PATCH] crypto: DRBG - guard uninstantion by lock

[Dmitry Vyukov]

On Mon, Apr 9, 2018 at 9:57 AM, Dmitry Vyukov  wrote:
> On Mon, Apr 9, 2018 at 7:40 AM, Stephan Mueller  wrote:
>> Am Montag, 9. April 2018, 00:46:03 CEST schrieb Theodore Y. Ts'o:
>>
>> Hi Theodore,
>>>
>>> So the syzbot will run while the patch goes through the normal e-mail
>>> review process, which is kind of neat.  :-)
>>
>> Thank you very much for the hint. That is a neat feature indeed.
>>
>> As I came late to the party and I missed the original mails, I am wondering
>> about which GIT repo was used and which branch of it. With that, I would be
>> happy to resubmit with the test line.
>
> All syzbot reported bugs are available here:
> https://groups.google.com/forum/#!searchin/syzkaller-bugs/"WARNING$20in$20kmem_cache_free;
> and here:
> https://syzkaller.appspot.com/
>
> But unfortunately testing won't work in this case, because I manually
> extracted a reproducer and syzbot does not know about it. This bug
> seems to lead to assorted silent heap corruptions and different
> manifestations each time, so it's difficult for syzbot to attribute a
> reproducer to the bug. When we debug it, it would be nice to
> understand why the heap corruption is silent and is not detected by
> KASAN and anything else, to prevent such unpleasant cases in future.
>
> I've tested it manually, but unfortunately kernel still crashed within a 
> minute:

Stephan,

Do you have any hypothesis as to why this is not detected by KASAN and
causes silent corruptions?
We generally try to understand such cases and improve KASAN so that it
catches such cases more reliably and they do not cause splashes of
random crashes on syzbot.

Thanks



> $ git status
> HEAD detached at f2d285669aae
> Changes not staged for commit:
>   (use "git add ..." to update what will be committed)
>   (use "git checkout -- ..." to discard changes in working directory)
>
> modified:   crypto/drbg.c
>
> $ git diff
> diff --git a/crypto/drbg.c b/crypto/drbg.c
> index 4faa2781c964..68c1949a253f 100644
> --- a/crypto/drbg.c
> +++ b/crypto/drbg.c
> @@ -1510,8 +1510,8 @@ static int drbg_instantiate(struct drbg_state
> *drbg, struct drbg_string *pers,
> return ret;
>
>  free_everything:
> -   mutex_unlock(>drbg_mutex);
> drbg_uninstantiate(drbg);
> +   mutex_unlock(>drbg_mutex);
> return ret;
>  }
>
> # ./a.out
> ...
> [  183.647874] FAULT_INJECTION: forcing a failure.
> [  183.647874] name failslab, interval 1, probability 0, space 0, times 0
> [  183.648287] Call Trace:
> [  183.648297]  dump_stack+0x1b9/0x29f
> [  183.648306]  ? arch_local_irq_restore+0x52/0x52
> [  183.648318]  ? __save_stack_trace+0x7e/0xd0
> [  183.651848]  should_fail.cold.4+0xa/0x1a
> [  183.652411]  ? fault_create_debugfs_attr+0x1f0/0x1f0
> [  183.653138]  ? kasan_kmalloc+0xc4/0xe0
> [  183.653694]  ? __kmalloc+0x14e/0x760
> [  183.654206]  ? drbg_kcapi_seed+0x776/0x12e0
> [  183.654798]  ? crypto_rng_reset+0x7c/0x130
> [  183.655379]  ? rng_setkey+0x25/0x30
> [  183.655882]  ? alg_setsockopt+0x306/0x3b0
> [  183.656450]  ? graph_lock+0x170/0x170
> [  183.656975]  ? entry_SYSENTER_compat+0x70/0x7f
> [  183.657606]  ? find_held_lock+0x36/0x1c0
> [  183.658164]  ? __lock_is_held+0xb5/0x140
> [  183.658728]  ? check_same_owner+0x320/0x320
> [  183.659321]  ? rcu_note_context_switch+0x710/0x710
> [  183.66]  should_failslab+0x124/0x180
> [  183.660561]  __kmalloc+0x2c8/0x760
> [  183.661046]  ? graph_lock+0x170/0x170
> [  183.661569]  ? drbg_kcapi_seed+0x882/0x12e0
> [  183.662161]  drbg_kcapi_seed+0x882/0x12e0
> [  183.662731]  ? drbg_seed+0x10a0/0x10a0
> [  183.663267]  ? lock_downgrade+0x8e0/0x8e0
> [  183.663833]  ? lock_acquire+0x1dc/0x520
> [  183.664385]  ? lock_release+0xa10/0xa10
> [  183.664934]  ? check_same_owner+0x320/0x320
> [  183.665530]  ? __sanitizer_cov_trace_const_cmp8+0x18/0x20
> [  183.666292]  ? __check_object_size+0x95/0x5d9
> [  183.666904]  ? sock_kmalloc+0x14e/0x1d0
> [  183.667444]  ? mark_held_locks+0xc9/0x160
> [  183.668020]  ? __might_sleep+0x95/0x190
> [  183.668567]  crypto_rng_reset+0x7c/0x130
> [  183.669124]  rng_setkey+0x25/0x30
> [  183.669598]  ? rng_sock_destruct+0x90/0x90
> [  183.670176]  alg_setsockopt+0x306/0x3b0
> [  183.670724]  __compat_sys_setsockopt+0x315/0x7c0
> [  183.671375]  ? __compat_sys_getsockopt+0x7f0/0x7f0
> [  183.672057]  ? __sanitizer_cov_trace_const_cmp4+0x16/0x20
> [  183.672813]  ? ksys_write+0x1a6/0x250
> [  183.67]  ? SyS_read+0x30/0x30
> [  183.673811]  compat_SyS_setsockopt+0x34/0x50
> [  183.674416]  ? scm_detach_fds_compat+0x440/0x440
> [  183.675079]  do_fast_syscall_32+0x41f/0x10dc
> [  183.675725]  ? do_page_fault+0xee/0x8a7
> [  183.676284]  ? do_int80_syscall_32+0xa70/0xa70
> [  183.676925]  ? trace_hardirqs_on_thunk+0x1a/0x1c
> [  183.677590]  ? __sanitizer_cov_trace_const_cmp4+0x16/0x20
> [  183.678348]  ? syscall_return_slowpath+0x30f/0x5c0
> [  183.679026]  ? sysret32_from_system_call+0x5/0x3c
> [  183.679694]  ? 

Re: [PATCH] crypto: DRBG - guard uninstantion by lock

[Dmitry Vyukov]

On Mon, Apr 9, 2018 at 7:40 AM, Stephan Mueller  wrote:
> Am Montag, 9. April 2018, 00:46:03 CEST schrieb Theodore Y. Ts'o:
>
> Hi Theodore,
>>
>> So the syzbot will run while the patch goes through the normal e-mail
>> review process, which is kind of neat.  :-)
>
> Thank you very much for the hint. That is a neat feature indeed.
>
> As I came late to the party and I missed the original mails, I am wondering
> about which GIT repo was used and which branch of it. With that, I would be
> happy to resubmit with the test line.

All syzbot reported bugs are available here:
https://groups.google.com/forum/#!searchin/syzkaller-bugs/"WARNING$20in$20kmem_cache_free;
and here:
https://syzkaller.appspot.com/

But unfortunately testing won't work in this case, because I manually
extracted a reproducer and syzbot does not know about it. This bug
seems to lead to assorted silent heap corruptions and different
manifestations each time, so it's difficult for syzbot to attribute a
reproducer to the bug. When we debug it, it would be nice to
understand why the heap corruption is silent and is not detected by
KASAN and anything else, to prevent such unpleasant cases in future.

I've tested it manually, but unfortunately kernel still crashed within a minute:

$ git status
HEAD detached at f2d285669aae
Changes not staged for commit:
  (use "git add ..." to update what will be committed)
  (use "git checkout -- ..." to discard changes in working directory)

modified:   crypto/drbg.c

$ git diff
diff --git a/crypto/drbg.c b/crypto/drbg.c
index 4faa2781c964..68c1949a253f 100644
--- a/crypto/drbg.c
+++ b/crypto/drbg.c
@@ -1510,8 +1510,8 @@ static int drbg_instantiate(struct drbg_state
*drbg, struct drbg_string *pers,
return ret;

 free_everything:
-   mutex_unlock(>drbg_mutex);
drbg_uninstantiate(drbg);
+   mutex_unlock(>drbg_mutex);
return ret;
 }

# ./a.out
...
[  183.647874] FAULT_INJECTION: forcing a failure.
[  183.647874] name failslab, interval 1, probability 0, space 0, times 0
[  183.648287] Call Trace:
[  183.648297]  dump_stack+0x1b9/0x29f
[  183.648306]  ? arch_local_irq_restore+0x52/0x52
[  183.648318]  ? __save_stack_trace+0x7e/0xd0
[  183.651848]  should_fail.cold.4+0xa/0x1a
[  183.652411]  ? fault_create_debugfs_attr+0x1f0/0x1f0
[  183.653138]  ? kasan_kmalloc+0xc4/0xe0
[  183.653694]  ? __kmalloc+0x14e/0x760
[  183.654206]  ? drbg_kcapi_seed+0x776/0x12e0
[  183.654798]  ? crypto_rng_reset+0x7c/0x130
[  183.655379]  ? rng_setkey+0x25/0x30
[  183.655882]  ? alg_setsockopt+0x306/0x3b0
[  183.656450]  ? graph_lock+0x170/0x170
[  183.656975]  ? entry_SYSENTER_compat+0x70/0x7f
[  183.657606]  ? find_held_lock+0x36/0x1c0
[  183.658164]  ? __lock_is_held+0xb5/0x140
[  183.658728]  ? check_same_owner+0x320/0x320
[  183.659321]  ? rcu_note_context_switch+0x710/0x710
[  183.66]  should_failslab+0x124/0x180
[  183.660561]  __kmalloc+0x2c8/0x760
[  183.661046]  ? graph_lock+0x170/0x170
[  183.661569]  ? drbg_kcapi_seed+0x882/0x12e0
[  183.662161]  drbg_kcapi_seed+0x882/0x12e0
[  183.662731]  ? drbg_seed+0x10a0/0x10a0
[  183.663267]  ? lock_downgrade+0x8e0/0x8e0
[  183.663833]  ? lock_acquire+0x1dc/0x520
[  183.664385]  ? lock_release+0xa10/0xa10
[  183.664934]  ? check_same_owner+0x320/0x320
[  183.665530]  ? __sanitizer_cov_trace_const_cmp8+0x18/0x20
[  183.666292]  ? __check_object_size+0x95/0x5d9
[  183.666904]  ? sock_kmalloc+0x14e/0x1d0
[  183.667444]  ? mark_held_locks+0xc9/0x160
[  183.668020]  ? __might_sleep+0x95/0x190
[  183.668567]  crypto_rng_reset+0x7c/0x130
[  183.669124]  rng_setkey+0x25/0x30
[  183.669598]  ? rng_sock_destruct+0x90/0x90
[  183.670176]  alg_setsockopt+0x306/0x3b0
[  183.670724]  __compat_sys_setsockopt+0x315/0x7c0
[  183.671375]  ? __compat_sys_getsockopt+0x7f0/0x7f0
[  183.672057]  ? __sanitizer_cov_trace_const_cmp4+0x16/0x20
[  183.672813]  ? ksys_write+0x1a6/0x250
[  183.67]  ? SyS_read+0x30/0x30
[  183.673811]  compat_SyS_setsockopt+0x34/0x50
[  183.674416]  ? scm_detach_fds_compat+0x440/0x440
[  183.675079]  do_fast_syscall_32+0x41f/0x10dc
[  183.675725]  ? do_page_fault+0xee/0x8a7
[  183.676284]  ? do_int80_syscall_32+0xa70/0xa70
[  183.676925]  ? trace_hardirqs_on_thunk+0x1a/0x1c
[  183.677590]  ? __sanitizer_cov_trace_const_cmp4+0x16/0x20
[  183.678348]  ? syscall_return_slowpath+0x30f/0x5c0
[  183.679026]  ? sysret32_from_system_call+0x5/0x3c
[  183.679694]  ? trace_hardirqs_off_thunk+0x1a/0x1c
[  183.680380]  entry_SYSENTER_compat+0x70/0x7f
[  183.681000] RIP: 0023:0xf7f0ecb9
[  183.681488] RSP: 002b:ffeb1e9c EFLAGS: 0296 ORIG_RAX:
016e
[  183.682606] RAX: ffda RBX: 0003 RCX: 0117
[  183.683620] RDX: 0001 RSI: 205b1fd0 RDI: 
[  183.684602] RBP: 2040 R08:  R09: 
[  183.685622] R10:  R11:  R12: 
[  183.686642] R13:  R14: 

Re: [PATCH] crypto: DRBG - guard uninstantion by lock

[Stephan Mueller]

Am Montag, 9. April 2018, 00:46:03 CEST schrieb Theodore Y. Ts'o:

Hi Theodore,
> 
> So the syzbot will run while the patch goes through the normal e-mail
> review process, which is kind of neat.  :-)

Thank you very much for the hint. That is a neat feature indeed.

As I came late to the party and I missed the original mails, I am wondering 
about which GIT repo was used and which branch of it. With that, I would be 
happy to resubmit with the test line.

Ciao
Stephan

Re: [PATCH] crypto: DRBG - guard uninstantion by lock

[Theodore Y. Ts'o]

On Sun, Apr 08, 2018 at 09:07:03PM +0200, Stephan Müller wrote:
> Can you please check whether the attached patch fixes the issue?
> 

Stephan,

FYI, if you incude in your e-mail "#syz test  " as
the first line of your patch and the syzbot e-mail is cc'ed, the
syzbot will automatically apply the patch in the e-mail against the
git tree/branch specified in the "#syz test" line, and then try to see
if the problem it discovered still reproduces --- and then send you
e-mail one way or another.

So the syzbot will run while the patch goes through the normal e-mail
review process, which is kind of neat.  :-)

Cheers,

- Ted

[PATCH] crypto: DRBG - guard uninstantion by lock

[Stephan Müller]

Am Sonntag, 8. April 2018, 17:41:17 CEST schrieb Dmitry Vyukov:

Hi Dmitry,
> 
> Hi,
> 
> Here is config and kernel commit:
> https://groups.google.com/d/msg/syzkaller-bugs/PINYyzoaG1s/ntZPOZdcCAAJ
> You can also find compiler and image here if necessary:
> https://github.com/google/syzkaller/blob/master/docs/syzbot.md
> 
> And note that the program needs to be compiled with -m32. The bugs is
> probably not-compat specific, but the program injects fault into a
> particular malloc invocation and maybe malloc numbering is affected by
> compat path.

I am unable to reproduce the issue. But since you mention that you induce 
errors, I could see that the unlocking of the DRBG context is too soon.

Can you please check whether the attached patch fixes the issue?

Thanks

---8<---

In the error code path, the uninstantiation must be guarded by a lock to
ensure that the modification of the context is fully atomic.

Signed-off-by: Stephan Mueller 
Reported-by: syzkaller
---
 crypto/drbg.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/crypto/drbg.c b/crypto/drbg.c
index 4faa2781c964..68c1949a253f 100644
--- a/crypto/drbg.c
+++ b/crypto/drbg.c
@@ -1510,8 +1510,8 @@ static int drbg_instantiate(struct drbg_state *drbg, 
struct drbg_string *pers,
return ret;
 
 free_everything:
-   mutex_unlock(>drbg_mutex);
drbg_uninstantiate(drbg);
+   mutex_unlock(>drbg_mutex);
return ret;
 }
 
-- 
2.14.3