linux-fscrypt.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Eric Biggers <ebiggers@kernel.org>
To: Doug Anderson <dianders@chromium.org>
Cc: Gwendal Grignou <gwendal@chromium.org>, Chao Yu <chao@kernel.org>,
	Ryo Hashimoto <hashimoto@chromium.org>,
	Vadim Sukhomlinov <sukhomlinov@google.com>,
	Guenter Roeck <groeck@chromium.org>,
	apronin@chromium.org, linux-doc@vger.kernel.org,
	Andreas Dilger <adilger.kernel@dilger.ca>,
	"Theodore Y. Ts'o" <tytso@mit.edu>,
	Jonathan Corbet <corbet@lwn.net>,
	LKML <linux-kernel@vger.kernel.org>,
	Jaegeuk Kim <jaegeuk@kernel.org>,
	linux-fscrypt@vger.kernel.org, linux-ext4@vger.kernel.org,
	linux-f2fs-devel@lists.sourceforge.net
Subject: Re: [PATCH] Revert "ext4 crypto: fix to check feature status before get policy"
Date: Thu, 31 Oct 2019 21:36:20 -0700	[thread overview]
Message-ID: <20191101043620.GA703@sol.localdomain> (raw)
In-Reply-To: <CAD=FV=URZX4t-TB2Ne8y5ZfeBGoyhsPZhcncQ0yPe3cRXi=1gw@mail.gmail.com>

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@chromium.org> wrote:
> >
> > Hi,
> >
> > On Wed, Oct 30, 2019 at 1:57 PM Eric Biggers <ebiggers@kernel.org> 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".)

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

  reply	other threads:[~2019-11-01  4:36 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-10-30 17:06 [PATCH] Revert "ext4 crypto: fix to check feature status before get policy" Douglas Anderson
2019-10-30 17:37 ` Eric Biggers
2019-10-30 17:51   ` Doug Anderson
2019-10-30 19:02     ` Eric Biggers
2019-10-30 20:57       ` Eric Biggers
2019-10-30 21:59         ` Doug Anderson
2019-10-31 17:52           ` Doug Anderson
2019-11-01  4:36             ` Eric Biggers [this message]
2019-11-01 13:32               ` Guenter Roeck
2019-11-01 18:17               ` Guenter Roeck
2019-11-02 22:10                 ` Guenter Roeck
2019-11-03 13:19                   ` Theodore Y. Ts'o
2019-11-04  7:45       ` Chao Yu

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20191101043620.GA703@sol.localdomain \
    --to=ebiggers@kernel.org \
    --cc=adilger.kernel@dilger.ca \
    --cc=apronin@chromium.org \
    --cc=chao@kernel.org \
    --cc=corbet@lwn.net \
    --cc=dianders@chromium.org \
    --cc=groeck@chromium.org \
    --cc=gwendal@chromium.org \
    --cc=hashimoto@chromium.org \
    --cc=jaegeuk@kernel.org \
    --cc=linux-doc@vger.kernel.org \
    --cc=linux-ext4@vger.kernel.org \
    --cc=linux-f2fs-devel@lists.sourceforge.net \
    --cc=linux-fscrypt@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=sukhomlinov@google.com \
    --cc=tytso@mit.edu \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).