Re: FW: Use-after-free while dm-verity

* Re: FW: Use-after-free while dm-verity
       [not found]   ` <20180816003535epcms2p6bba40fd6423d0bb605b4dec3866f84c9@epcms2p6>
@ 2018-08-16  0:57     ` Gilad Ben-Yossef
       [not found]       ` <20180816015056epcms2p8954563f824dc617ae13ab1364715159a@epcms2p8>
       [not found]     ` <CGME20180814083408epcms2p1980005414da7197755f798aa9ed47a4f@epcms2p8>
  1 sibling, 1 reply; 8+ messages in thread
From: Gilad Ben-Yossef @ 2018-08-16  0:57 UTC (permalink / raw)
  To: hyeon.chung; +Cc: dm-devel, 김호성

[-- Attachment #1.1.1: Type: text/plain, Size: 6115 bytes --]

Hi Hyeon,

On Thu, Aug 16, 2018 at 3:35 AM, 정혜연 <hyeon.chung@samsung.com> wrote:

> Hi,
>
>
>
> We found some async commit and are wondering if this could be make this
> issue or not.
>
> I want to test after reverting this but I'm not sure it is ok to revert
> only this commit and when I revert it there's a conflict.
>
>
> Could you please help me?
>

I am on vacation, so please excuse any delay in handling this. Luckily I
have my laptop with me and will try to do me best to help.

The description below relates to "use-after-free". I just want to make sure
I understand: does this relate to the memcpy after unmap, right?

Normally, I would say that since we use kmap_atomic to map the data,
preemption should be disabled and scheduling should not happen, I believe.

I will only be able to take a look at the code in the evening, but in the
mean time, can you verify the area the memcpy is applied to is mapped with
kmap_atomic?

thanks!
Gilad

>
> commit d1ac3ff008fb9a48f91fc15920b4c8db24c0f03e
> Author: Gilad Ben-Yossef <gilad@benyossef.com>
> Date:   Sun Feb 19 14:46:07 2017 +0200
>
>     dm verity: switch to using asynchronous hash crypto API
>
>     Use of the synchronous digest API limits dm-verity to using pure
>     CPU based algorithm providers and rules out the use of off CPU
>     algorithm providers which are normally asynchronous by nature,
>     potentially freeing CPU cycles.
>
>     This can reduce performance per Watt in situations such as during
>     boot time when a lot of concurrent file accesses are made to the
>     protected volume.
>
>     Signed-off-by: Gilad Ben-Yossef <gilad@benyossef.com>
>     CC: Eric Biggers <ebiggers3@gmail.com>
>     CC: Ondrej Mosn찼훾ek <omosnacek+linux-crypto@gmail.com>
>     Tested-by: Milan Broz <gmazyland@gmail.com>
>     Signed-off-by: Mike Snitzer <snitzer@redhat.com>
>
>
>
>
>
> 감사합니다.
>
> 정혜연 드림
>
>
>
>
>
> --------- *Original Message* ---------
>
> *Sender* : 정혜연 <hyeon.chung@samsung.com> Principal
> Engineer/Platform개발팀(S.LSI)/삼성전자
>
> *Date* : 2018-08-14 17:34 (GMT+9)
>
> *Title* : Use-after-free while dm-verity
>
>
>
> Hi,
>
> I'm a samsung engineer who is responsible for dm-verity.
>
>
>
> We just enabled dm-verity for android verfied boot 2.0 (using linux
> 4.14.56).
>
> After enabling, we met kernel panic issue caused by dm-verity frequently
> and it always showed same call stack like below.
>
>
>
> Could you please let me know If you know any similar issue or have any
> opinion?
>
> If this mail list is not right, please forward proper person or let me
> know.
>
>
>
> Thanks in advance.
>
>
>
> < 4>[ 4686.384679]  [5:  kworker/u16:1:   58] Workqueue: kverityd
> verity_work
> < 4>[ 4686.384691]  [5:  kworker/u16:1:   58] task: ffffffc8742a0080
> task.stack: ffffff800b1d8000
> < 4>[ 4686.384705]  [5:  kworker/u16:1:   58] PC is at __memcpy+0x70/0x180
> < 4>[ 4686.384718]  [5:  kworker/u16:1:   58] LR is at
> crypto_sha1_update+0x94/0xe4
>
> ...
>
> <4>[ 4686.387488]  [5:  kworker/u16:1:   58] [<ffffff8008b07370>]
> __memcpy+0x70/0x180
> < 4>[ 4686.387503]  [5:  kworker/u16:1:   58] [<ffffff80083a0f6c>]
> crypto_shash_update+0x2c/0x34
> < 4>[ 4686.387514]  [5:  kworker/u16:1:   58] [<ffffff80083a12c0>]
> shash_ahash_update+0x3c/0x80
> < 4>[ 4686.387525]  [5:  kworker/u16:1:   58] [<ffffff80083a1638>]
> shash_async_update+0x10/0x18
> < 4>[ 4686.387538]  [5:  kworker/u16:1:   58] [<ffffff8008824974>]
> verity_hash_update+0x50/0xdc
> < 4>[ 4686.387549]  [5:  kworker/u16:1:   58] [<ffffff80088247c0>]
> verity_hash+0x58/0xa0
> < 4>[ 4686.387560]  [5:  kworker/u16:1:   58] [<ffffff8008824c98>]
> verity_verify_level+0xc4/0x190
> < 4>[ 4686.387571]  [5:  kworker/u16:1:   58] [<ffffff8008824bc4>]
> verity_hash_for_block+0xf0/0x100
> < 4>[ 4686.387581]  [5:  kworker/u16:1:   58] [<ffffff80088244c8>]
> fec_read_bufs+0x144/0x30c
> < 4>[ 4686.387592]  [5:  kworker/u16:1:   58] [<ffffff8008823730>]
> fec_decode_rsb+0x170/0x4bc
> < 4>[ 4686.387603]  [5:  kworker/u16:1:   58] [<ffffff8008823524>]
> verity_fec_decode+0xe8/0x184
> < 4>[ 4686.387613]  [5:  kworker/u16:1:   58] [<ffffff8008824d38>]
> verity_verify_level+0x164/0x190
> < 4>[ 4686.387623]  [5:  kworker/u16:1:   58] [<ffffff8008824bc4>]
> verity_hash_for_block+0xf0/0x100
> < 4>[ 4686.387634]  [5:  kworker/u16:1:   58] [<ffffff80088264cc>]
> verity_work+0xf8/0x308
> < 4>[ 4686.387646]  [5:  kworker/u16:1:   58] [<ffffff80080cb5ec>]
> process_one_work+0x2d4/0x608
> < 4>[ 4686.387656]  [5:  kworker/u16:1:   58] [<ffffff80080cbbf4>]
> worker_thread+0x220/0x340
> < 4>[ 4686.387667]  [5:  kworker/u16:1:   58] [<ffffff80080d0498>]
> kthread+0x11c/0x12c
> < 4>[ 4686.387678]  [5:  kworker/u16:1:   58] [<ffffff800808557c>]
> ret_from_fork+0x10/0x18
> < 0>[ 4686.387690]  [5:  kworker/u16:1:   58] Code: 54000080 540000ab
> a8c12027 a88120c7 (a8c12027)
> < 4>[ 4686.387839]  [5:  kworker/u16:1:   58] ---[ end trace
> 54664349b0f9422c ]---
>
>
>
>
>
> In memcopy function,
>
> load x1 register successfully then context switched by scheduler.
>
> After returning to same memcopy function, failed another x1 load becuase
> this is unmapped.(walk->data)
>
> <1>[  889.732488]  [2:  kworker/u16:3:15332] Unable to handle kernel
> paging request at virtual address ffffffc00b6ad000
>
>
>
> I suspect that this is not protected propely becuase callstack and problem
> (walk->data: use-after-free) is alwasys same.
>
> 1. shrink_node is executed on onther core at that time. Is it
> possible this make this problem?
>
> 2. I found that walk->data is unmapped by ahahs.c but I don't know whether
> this cause this unmapped situation or not.
>
> if (walk->flags & CRYPTO_ALG_ASYNC)
>     kunmap(walk->pg);
> else
>     kunmap_atomic(walk->data);
>
>
>
>
>
> Best Regards,
>
> HyeYeon Chung
>
>
>
>
>
>
>

-- 
Gilad Ben-Yossef
Chief Coffee Drinker

values of β will give rise to dom!

[-- Attachment #1.1.2: Type: text/html, Size: 10554 bytes --]

[-- Attachment #1.2: Type: image/gif, Size: 13402 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

^ permalink raw reply	[flat|nested] 8+ messages in thread