[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



I checked the flag by printing the value and it didn't have CRYPTO_ALG_ASYNC.

And we already set CONFIG_DEBUG_PREEMPT to true but no notible log.

 

We're keep tracing any possibility, and I'll share we get any useful information or suspected situation.

 

BRs,

Hyeyeon Chung

 

 

--------- Original Message ---------

Sender : Gilad Ben-Yossef <gilad benyossef com>

Date : 2018-08-17 01:04 (GMT+9)

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

 

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 = "" =
> 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 = ""
>  else
>   walk->data = ""
>  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!

 

 


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]