On Thu, Oct 31, 2019 at 9:36 PM Eric Biggers <ebiggers@xxxxxxxxxx> wrote: > > On Thu, Oct 31, 2019 at 10:52:19AM -0700, Doug Anderson wrote: > > Hi, > > > > On Wed, Oct 30, 2019 at 2:59 PM Doug Anderson <dianders@xxxxxxxxxxxx> wrote: > > > > > > Hi, > > > > > > On Wed, Oct 30, 2019 at 1:57 PM Eric Biggers <ebiggers@xxxxxxxxxx> wrote: > > > > > > > > FWIW, from reading the Chrome OS code, I think the code you linked to isn't > > > > where the breakage actually is. I think it's actually at > > > > https://chromium.googlesource.com/chromiumos/platform2/+/refs/heads/master/chromeos-common-script/share/chromeos-common.sh#375 > > > > ... where an init script is using the error message printed by 'e4crypt > > > > get_policy' to decide whether to add -O encrypt to the filesystem or not. > > > > > > > > It really should check instead: > > > > > > > > [ -e /sys/fs/ext4/features/encryption ] > > > > > > OK, I filed <https://crbug.com/1019939> and CCed all the people listed > > > in the cryptohome "OWNERS" file. Hopefully one of them can pick this > > > up as a general cleanup. Thanks! > > > > Just to follow-up: I did a quick test here to see if I could fix > > "chromeos-common.sh" as you suggested. Then I got rid of the Revert > > and tried to login. No joy. > > > > Digging a little deeper, the ext4_dir_encryption_supported() function > > is called in two places: > > * chromeos-install > > * chromeos_startup > > > > In my test case I had a machine that I'd already logged into (on a > > previous kernel version) and I was trying to log into it a second > > time. Thus there's no way that chromeos-install could be involved. > > Looking at chromeos_startup: > > > > https://chromium.googlesource.com/chromiumos/platform2/+/refs/heads/master/init/chromeos_startup > > > > ...the function is only used for setting up the "encrypted stateful" > > partition. That wasn't where my failure was. My failure was with > > logging in AKA with cryptohome. Thus I think it's plausible that my > > original commit message pointing at cryptohome may have been correct. > > It's possible that there were _also_ problems with encrypted stateful > > that I wasn't noticing, but if so they were not the only problems. > > > > It still may be wise to make Chrome OS use different tests, but it > > might not be quite as simple as hoped... > > > > Ah, I think I found it: > > https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/2cbdedd5eca0a57d9596671a99da5fab8e60722b/sys-apps/upstart/files/upstart-1.2-dircrypto.patch > > The init process does EXT4_IOC_GET_ENCRYPTION_POLICY on /, and if the error is > EOPNOTSUPP, it skips creating the "dircrypto" keyring. So then cryptohome can't > add keys later. (Note the error message you got, "Error adding dircrypto key".) > Good catch. I'll try replacing that with a check for the sysfs flag and see if that does the trick. Guenter > So it looks like the kernel patch broke both that and > ext4_dir_encryption_supported(). > > I don't see how it could have broken cryptohome by itself, since AFAICS > cryptohome only uses EXT4_IOC_GET_ENCRYPTION_POLICY on the partition which is > supposed to have the 'encrypt' feature set. > > - Eric
