On Thu, 2018-12-20 at 17:56 +0200, Amir Goldstein wrote: > On Thu, Dec 20, 2018 at 4:20 PM Mimi Zohar <zohar@xxxxxxxxxxxxx> wrote: > > > > On Thu, 2018-12-20 at 14:04 +0200, Amir Goldstein wrote: > > > > > > > 2. What does the call stack look like when failing to verify the hash > > > > > on oo.text? > > > > > > > > *** Cat of same file on merged dir fails (ino != 258) > > > > cat: overlay/merged/abc.text: Permission denied > > > > > > > > [ 476.770869] evm: ino: 38593 258 38593 abc.text > > > > [ 476.770876] evm: ino: 38593 > > > > [ 476.770883] CPU: 3 PID: 3928 Comm: cat Not tainted 4.20.0-rc2+ #1287 > > > > [ 476.770887] Hardware name: LENOVO 20BTS1NJ00/20BTS1NJ00, BIOS N14ET48W (1.26 ) 06/11/2018 > > > > [ 476.770890] Call Trace: > > > > [ 476.770906] dump_stack+0x46/0x5b > > > > [ 476.770913] hmac_add_misc+0x171/0x180 > > > > [ 476.770920] evm_calc_hmac_or_hash+0x1c9/0x280 > > > > [ 476.770927] evm_verify_hmac+0x11f/0x2b0 > > > > [ 476.770933] ? evm_protected_xattr+0x6c/0x90 > > > > [ 476.770940] ima_appraise_measurement+0x83/0x510 > > > > [ 476.770948] process_measurement+0x646/0x6f0 > > > > [ 476.770955] ? selinux_file_open+0xa8/0xc0 > > > > [ 476.770961] ? do_dentry_open+0x25c/0x340 > > > > [ 476.770966] ? open_with_fake_path+0x48/0x70 > > > > [ 476.770974] ? ovl_open_realfile+0x56/0xe0 > > > > [ 476.770981] ima_file_check+0x4a/0x60 > > > > > > If you let this check return success even though appraisal failed, > > > you will see that ovl_open_realfile() will end up calling ima_file_check() > > > again with the "real" file and this check should not fail. > > > Hence, my suggestion to mark the overlayfs sb with SB_NOIMA. > > > > There's all sorts of caching of results involved in both EVM and IMA. > > Not so easy to modify. Assuming that I properly removed the caching, > > I'm not seeing another call to ima_file_check. > > > > Mmm right. open_with_fake_path() doesn't call ima_file_check(). My bad. > > My intuition is that all d_backing_inode() calls under security/integrity > should be converted to d_real_inode(). After all, users of d_backing_inode() > probably want the lower/upper inode, because that is what the helper > claims to do. The reason things worked in v4.18 is because > d_backing_inode(file_dentry(file)) was the real inode. > > My intuition is that this statement is true for other subsystems that > use d_backing_inode() as well. We could have changed the > implementation of d_backing_inode() to call d_real_inode() > to potentially fix all those subsystems with less churn, but it is hard to > audit all caller to make sure this is correct for all of them. > In fact, it is probably not correct for code in fs/*.c. > > So the safer approach is to fix one subsystem at a time by converting > callers to d_real_inode() and in the end get rid of d_backing_inode(). > > Mimi, please try if this fixes EVM over overlayfs. Yes, changing the hmac_add_misc() call in evm_calc_hmac_or_hash() fixes the problem. Mimi > > Miklos, do you have other thoughts about how to address this issue? > > Thanks, > Amir. >
