<HTML><HEAD> <META content=IE=5 http-equiv=X-UA-Compatible> <META content="text/html; charset=utf-8" http-equiv=Content-Type> <STYLE id=mysingle_style type=text/css>.search-word { BACKGROUND-COLOR: #ffee94 } P { MARGIN-BOTTOM: 5px; FONT-SIZE: 10pt; FONT-FAMILY: 맑은 고딕, arial; MARGIN-TOP: 5px } TD { MARGIN-BOTTOM: 5px; FONT-SIZE: 10pt; FONT-FAMILY: 맑은 고딕, arial; MARGIN-TOP: 5px } LI { MARGIN-BOTTOM: 5px; FONT-SIZE: 10pt; FONT-FAMILY: 맑은 고딕, arial; MARGIN-TOP: 5px } BODY { FONT-SIZE: 10pt; FONT-FAMILY: 맑은 고딕, arial; MARGIN: 10px; LINE-HEIGHT: 1.4 } </STYLE> <STYLE id=knox_style type=text/css>P { MARGIN-BOTTOM: 5px; FONT-SIZE: 10pt; FONT-FAMILY: 맑은 고딕, arial; MARGIN-TOP: 5px } </STYLE> <META name=GENERATOR content=ActiveSquare></HEAD> <BODY style="OVERFLOW: auto"> 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 <PRE>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! </PRE><table id=bannersignimg data-cui-lock="true" namo_lock><tr><td> </td></tr></table><table id=confidentialsignimg data-cui-lock="true" namo_lock><tr><td><img style="border: 0px solid currentColor; border-image: none; width: 520px; height: 144px; display: inline-block;" unselectable="on" data-cui-image="true" src="cid:cafe_image_0@s-core.co.kr"> </td></tr></table></BODY></HTML><img src='http://ext.samsung.net/mail/ext/v1/external/status/update?userid=hyeon.chung&do=bWFpbElEPTIwMTgwODIwMDUyNjUyZXBjbXMycDRiMjNjYjAyMmYzNDQ3NDM1Y2ZkMTEyYmE2OWQ1MGMyNyZyZWNpcGllbnRBZGRyZXNzPWRtLWRldmVsQHJlZGhhdC5jb20_' border=0 width=0 height=0 style='display:none'>