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

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

[Gilad Ben-Yossef]

[-- 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 --]

Fwd: (2) FW: Use-after-free while dm-verity

[Gilad Ben-Yossef]

I missed sending my reply to the list by myself mistake so forwarding.
---------- Forwarded message ----------
From: Gilad Ben-Yossef <gilad@benyossef.com>
Date: Thu, 16 Aug 2018 09:48:19 +0700
Subject: Re: (2) FW: Use-after-free while dm-verity
To: hyeon.chung@samsung.com

On 8/16/18, 정혜연 <hyeon.chung@samsung.com> wrote:
> Thank you for reply and sorry for inturrupting your vacation.
>

Eh, don't worry about it. I enjoy kernel development work. Just don't
tell my wife... :-)
>
>
> I guess kunmap_atomic / kmap_atomic could be protected from scheduling but
> the operation using the resource like memcopy cannot be protected, right?
>
> Please let me know if I was wrong.

kmap_atomic() disables preemption until the kunmap operation and so
scheduling should not be possible while the memory is mapped.


>
>
>
> I am testing after reverting and resolving confict now. But as I said before
> I'm not sure it is ok just revert only this one.
>
> Is it ok?

The patch stands on its own so that should be ok if that patch is the
root cause of the problem bit if the problem is some where else it
could also be hiding the problem so it would be better to understand
what is going on first or it could bite you elsewhere.

Gilad
>
>
>
> Best Regards,
>
> Hyeyeon Chung
>
>
>
> --------- Original Message ---------
>
> Sender : Gilad Ben-Yossef <gilad@benyossef.com>
>
> Date : 2018-08-16 09:57 (GMT+9)
>
> Title : Re: FW: Use-after-free while dm-verity
>
>
>
> 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
>>
>>
>>
>>
>>
>> [image]
>
>
>
>
> --
> Gilad Ben-Yossef
> Chief Coffee Drinker
>
> values of β will give rise to dom!
>
>
>
> [image]
>
> [image]


-- 
Gilad Ben-Yossef
Chief Coffee Drinker

values of β will give rise to dom!



-- 
Gilad Ben-Yossef
Chief Coffee Drinker

values of β will give rise to dom!

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Re: FW: RE: FW: (2) FW: Use-after-free while dm-verity

[Gilad Ben-Yossef]

On Thu, Aug 16, 2018 at 11:08 AM, 정혜연 <hyeon.chung@samsung.com> wrote:
> Dear Gilad,
>
>
>
> I'm sorry I'm confused call sequence.
>
> calling function may be not crypto_ahash_walk_first but
> crypto_hash_walk_first.
>
> I don't know how code reaches there (verity_hash_update --> ??? ->
> shash_async_update)but below call stack showed like this.
>

Basically through an inline function that calls shash_async_update via
 function pointer.

>
>
> int crypto_hash_walk_first(struct ahash_request *req,
>       struct crypto_hash_walk *walk)
> {
>  walk->total = req->nbytes;
>
>  if (!walk->total) {
>   walk->entrylen = 0;
>   return 0;
>  }
>
>  walk->alignmask = crypto_ahash_alignmask(crypto_ahash_reqtfm(req));
>  walk->sg = req->src;
>  walk->flags = req->base.flags & CRYPTO_TFM_REQ_MASK;
>
>  return hash_walk_new_entry(walk);
> }
>
> So, is it right that walk->flag doesn't have  CRYPTO_ALG_ASYNC ?
>

Yes, I believe any shash implementation (which this is, since I'm
seeing *shash* function in the call stack) would not have
CRYPTO_ALG_ASYNC  in walk->flag.
I'm guessing you are using the SHA1 Arm CE implementation, right? this
is a synchronous implementation and would not have that flag set.
It would be nice if you can verify this assumption, say by planting a
printk to print the flag but I'm pretty sure of that.

This means two things:

1. There should never be a schedule call in between the kmap_atomic()
and kunmap_atomic() since kmap_atomic() disables preemption. The fact
that you're seeing one points to something highly irregular.

2. If there is a schedule between the two, there is no wonder the
mapping is lost, kmap_atomic is designed to be, well... atomic.

Could it be you have some other, possibly unrelated, code or driver
that has a bug of enabling preemption one two many? are you using any
out of tree drivers or patches with this kernel?

Can you please compile the kernel with CONFIG_DEBUG_PREEMPT set (it's
in Kernel Hacking section of menuconfig)? that should catch any such
code.

If this doesn't bring anything to light, please provide a the full
kernel panic log (not just the snippet here), your kernel config and
what architecture/board you are running this on, as well as details
about DM-verity partition setup, size and underlying storage so I may
attempt to recreate this.

Cheers,
Gilad


>
>
> --------- Original Message ---------
>
> Sender : 정혜연 <hyeon.chung@samsung.com> Principal
> Engineer/Platform개발팀(S.LSI)/삼성전자
>
> Date : 2018-08-16 14:04 (GMT+9)
>
> Title : RE: FW: (2) FW: Use-after-free while dm-verity
>
>
>
> Thanks for your kind understanding and help :)
>
>
>
> Glance at the ahash,
>
> it seemed that ahash does not use kmap/unmap_atomic becuase of
> CRYPTO_ALG_ASYNC flag.
>
> As we analyzed abnormal situation, memcopy in verity_work was preempted by
> 20ms.
>
>
>
> < Task  >
>
> 1. time = 889711041874, sp = 18446743524145741088, task = 0xFFFFFFC8DC229B00
> = end+0x48D19DBB00, task_comm = "kworker/u16:3", pid = 15332),
> 2. time = 889732425104, sp = 18446743524145740192, task = 0xFFFFFFC8DC229B00
> = end+0x48D19DBB00, task_comm = "kworker/u16:3", pid = 15332),
>
> < Work >
> 3. time = 889711100643, sp = 18446743524145741072, worker =
> 0xFFFFFFC03A09F380 = end+0x402F851380, fn = 0xFFFFFF80087DE720 =
> verity_work, task_comm = "kworker/u16:3", en = 1),
>
> < IRQ >
> 4. time = 889711657451, sp = 18446743524088036864, irq = 9, fn =
> 0xFFFFFF800880D210 = exynos4_mct_tick_isr, action = 0xFFFFFFC8F5E83500 =
> end+0x48EB635500, en = 1),
>
>
>
>
>
> int crypto_ahash_walk_first(struct ahash_request *req,
>        struct crypto_hash_walk *walk)
> {
>  walk->total = req->nbytes;
>
>
>
>  if (!walk->total) {
>   walk->entrylen = 0;
>   return 0;
>  }
>
>  walk->alignmask = crypto_ahash_alignmask(crypto_ahash_reqtfm(req));
>  walk->sg = req->src;
>  walk->flags = req->base.flags & CRYPTO_TFM_REQ_MASK;
>  walk->flags |= CRYPTO_ALG_ASYNC;
>
>  BUILD_BUG_ON(CRYPTO_TFM_REQ_MASK & CRYPTO_ALG_ASYNC);
>
>  return hash_walk_new_entry(walk);
> }
>
>
>
> static int hash_walk_next(struct crypto_hash_walk *walk)
> {
>  unsigned int alignmask = walk->alignmask;
>  unsigned int offset = walk->offset;
>  unsigned int nbytes = min(walk->entrylen,
>       ((unsigned int)(PAGE_SIZE)) - offset);
>
>  if (walk->flags & CRYPTO_ALG_ASYNC)
>   walk->data = kmap(walk->pg);
>  else
>   walk->data = kmap_atomic(walk->pg);
>  walk->data += offset;
>
>  if (offset & alignmask) {
>   unsigned int unaligned = alignmask + 1 - (offset & alignmask);
>
>   if (nbytes > unaligned)
>    nbytes = unaligned;
>  }
>
>  walk->entrylen -= nbytes;
>  return nbytes;
> }
>
>
>
> int crypto_hash_walk_done(struct crypto_hash_walk *walk, int err)
> {
>  unsigned int alignmask = walk->alignmask;
>  unsigned int nbytes = walk->entrylen;
>
>  walk->data -= walk->offset;
>
>  if (nbytes && walk->offset & alignmask && !err) {
>   walk->offset = ALIGN(walk->offset, alignmask + 1);
>   nbytes = min(nbytes,
>         ((unsigned int)(PAGE_SIZE)) - walk->offset);
>   walk->entrylen -= nbytes;
>
>   if (nbytes) {
>    walk->data += walk->offset;
>    return nbytes;
>   }
>  }
>
>  if (walk->flags & CRYPTO_ALG_ASYNC)
>   kunmap(walk->pg);
>  else {
>   kunmap_atomic(walk->data);
>   /*
>    * The may sleep test only makes sense for sync users.
>    * Async users don't need to sleep here anyway.
>    */
>   crypto_yield(walk->flags);
>  }
>
>  if (err)
>   return err;
>
>  if (nbytes) {
>   walk->offset = 0;
>   walk->pg++;
>   return hash_walk_next(walk);
>  }
>
>  if (!walk->total)
>   return 0;
>
>  walk->sg = sg_next(walk->sg);
>
>  return hash_walk_new_entry(walk);
> }
>
>
>
> --------- Original Message ---------
>
> Sender : Gilad Ben-Yossef <gilad@benyossef.com>
>
> Date : 2018-08-16 11:53 (GMT+9)
>
> Title : Fwd: (2) FW: Use-after-free while dm-verity
>
>
>
> I missed sending my reply to the list by myself mistake so forwarding.
> ---------- Forwarded message ----------
> From: Gilad Ben-Yossef <gilad@benyossef.com>
> Date: Thu, 16 Aug 2018 09:48:19 +0700
> Subject: Re: (2) FW: Use-after-free while dm-verity
> To: hyeon.chung@samsung.com
>
> On 8/16/18, 정혜연 <hyeon.chung@samsung.com> wrote:
>> Thank you for reply and sorry for inturrupting your vacation.
>>
>
> Eh, don't worry about it. I enjoy kernel development work. Just don't
> tell my wife... :-)
>>
>>
>> I guess kunmap_atomic / kmap_atomic could be protected from scheduling but
>> the operation using the resource like memcopy cannot be protected, right?
>>
>> Please let me know if I was wrong.
>
> kmap_atomic() disables preemption until the kunmap operation and so
> scheduling should not be possible while the memory is mapped.
>
>
>>
>>
>>
>> I am testing after reverting and resolving confict now. But as I said
>> before
>> I'm not sure it is ok just revert only this one.
>>
>> Is it ok?
>
> The patch stands on its own so that should be ok if that patch is the
> root cause of the problem bit if the problem is some where else it
> could also be hiding the problem so it would be better to understand
> what is going on first or it could bite you elsewhere.
>
> Gilad
>>
>>
>>
>> Best Regards,
>>
>> Hyeyeon Chung
>>
>>
>>
>> --------- Original Message ---------
>>
>> Sender : Gilad Ben-Yossef <gilad@benyossef.com>
>>
>> Date : 2018-08-16 09:57 (GMT+9)
>>
>> Title : Re: FW: Use-after-free while dm-verity
>>
>>
>>
>> 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
>>>
>>>
>>>
>>>
>>>
>>> [image]
>>
>>
>>
>>
>> --
>> Gilad Ben-Yossef
>> Chief Coffee Drinker
>>
>> values of β will give rise to dom!
>>
>>
>>
>> [image]
>>
>> [image]
>
>
> --
> Gilad Ben-Yossef
> Chief Coffee Drinker
>
> values of β will give rise to dom!
>
>
>
> --
> Gilad Ben-Yossef
> Chief Coffee Drinker
>
> values of β will give rise to dom!
>
>
>
>



-- 
Gilad Ben-Yossef
Chief Coffee Drinker

values of β will give rise to dom!

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Re: (2) FW: RE: FW: (2) FW: Use-after-free while dm-verity

[정혜연]

[-- Attachment #1.1: Type: text/html, Size: 20820 bytes --]

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

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

Re: FW: RE:(2) FW: RE: FW: (2) FW: Use-after-free while dm-verity

[Gilad Ben-Yossef]

Hi,

On Mon, Aug 20, 2018 at 2:33 PM, 정혜연 <hyeon.chung@samsung.com> wrote:
>
> Dear Gilad,
>
>
>
> We found memcopy is not preempted as you explained.
>
> Memcopy make kernel panic at the very first time.
>
> Your comment was very helpful us to find this. Thank you.
>
>


I'm glad I could help.

>
> We found the other core executed shrink_node at that time, so we suspect that lowmem situation coulde make this problem.
>
> And we tested after reverting async patch for 3 days and there's no isuue so far.
>
> So we assume there's unexpected free in dm-verity or some page memory operation (ex. offset ?).
>
>
>
> Do you have any idea?
>
>


One thing that comes to mind is understanding why there are data
errors in the data being read. The stack trace you sent shows calls to
verity_fec_decode().
This would happen if there was a mismatch between the computed hash
and the expected hash of (in this specific case per the stack trace) a
storage block containing hashes for the data. It is therefore attempts
to fix the errors via employing Read Solomon codes.

While it is part of DM-Verity design, I'm surprised to see this happen
in a lab environment. Do you deliberately create 1 bit errors in the
data as a test or is this some coincidence?

Other than that, I return to my previous request:

"If this doesn't bring anything to light, please provide a the full
kernel panic log (not just the snippet here), your kernel config and
what architecture/board you are running this on, as well as details
about DM-verity partition setup, size and underlying storage so I may
attempt to recreate this."


This will best help me help you.


Cheers,

Gilad

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Re: FW: RE:(2) FW: RE: FW: (2) FW: Use-after-free while dm-verity

[Mike Snitzer]

On Tue, Aug 21 2018 at  5:38am -0400,
Gilad Ben-Yossef <gilad@benyossef.com> wrote:

> Hi,
> 
> On Mon, Aug 20, 2018 at 2:33 PM, 정혜연 <hyeon.chung@samsung.com> wrote:
> >
> > Dear Gilad,
> >
> >
> >
> > We found memcopy is not preempted as you explained.
> >
> > Memcopy make kernel panic at the very first time.
> >
> > Your comment was very helpful us to find this. Thank you.
> >
> >
> 
> 
> I'm glad I could help.
> 
> >
> > We found the other core executed shrink_node at that time, so we suspect that lowmem situation coulde make this problem.
> >
> > And we tested after reverting async patch for 3 days and there's no isuue so far.
> >
> > So we assume there's unexpected free in dm-verity or some page memory operation (ex. offset ?).
> >
> >
> >
> > Do you have any idea?
> >
> >
> 
> 
> One thing that comes to mind is understanding why there are data
> errors in the data being read. The stack trace you sent shows calls to
> verity_fec_decode().
> This would happen if there was a mismatch between the computed hash
> and the expected hash of (in this specific case per the stack trace) a
> storage block containing hashes for the data. It is therefore attempts
> to fix the errors via employing Read Solomon codes.
> 
> While it is part of DM-Verity design, I'm surprised to see this happen
> in a lab environment. Do you deliberately create 1 bit errors in the
> data as a test or is this some coincidence?
> 
> Other than that, I return to my previous request:
> 
> "If this doesn't bring anything to light, please provide a the full
> kernel panic log (not just the snippet here), your kernel config and
> what architecture/board you are running this on, as well as details
> about DM-verity partition setup, size and underlying storage so I may
> attempt to recreate this."
> 
> 
> This will best help me help you.

Can you please see if this patch helps?

https://patchwork.kernel.org/patch/10573051/

Thanks,
Mike

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Re: FW: RE:(2) FW: RE: FW: (2) FW: Use-after-free while dm-verity

[Gilad Ben-Yossef]

Hi HyeYeon,


On Wed, Aug 22, 2018 at 8:28 PM, Mike Snitzer <snitzer@redhat.com> wrote:

>
> Can you please see if this patch helps?
>
> https://patchwork.kernel.org/patch/10573051/
>

I'm curious if the patch pointed to by Mike resolved your issue :-)

Gilad


-- 
Gilad Ben-Yossef
Chief Coffee Drinker

values of β will give rise to dom!

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Re: (2) FW: RE:(2) FW: RE: FW: (2) FW: Use-after-free while dm-verity

[Gilad Ben-Yossef]

On Mon, Sep 3, 2018 at 9:54 AM, 정혜연 <hyeon.chung@samsung.com> wrote:
>
> Hi Gilad,
>
>
>
> I missed test slot because our test milestone is already ended so I have to wait kernel minor up for applying his patch (aosp common kernel).
>
> But I tested by myself to print vmalloc in case of is_vmalloc_addr(data) and dm corrupted right after printing vmalloc in lowmem situation.
>
> So I think that Mike's patch would be resolve this issue.
>
>
>
> We will keep tracking this issue and update you if any.


Great!

Thanks,
Gilad

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel