Linux-mtd Archive on lore.kernel.org
 help / color / Atom feed
From: "Theodore Y. Ts'o" <tytso@mit.edu>
To: linux-fscrypt@vger.kernel.org, linux-fsdevel@vger.kernel.org,
	linux-ext4@vger.kernel.org,
	linux-f2fs-devel@lists.sourceforge.net,
	linux-mtd@lists.infradead.org, linux-api@vger.kernel.org,
	linux-crypto@vger.kernel.org, keyrings@vger.kernel.org,
	Paul Crowley <paulcrowley@google.com>,
	Satya Tangirala <satyat@google.com>
Subject: Re: [f2fs-dev] [PATCH v7 07/16] fscrypt: add FS_IOC_REMOVE_ENCRYPTION_KEY ioctl
Date: Thu, 1 Aug 2019 01:31:08 -0400
Message-ID: <20190801053108.GD2769@mit.edu> (raw)
In-Reply-To: <20190801011140.GB687@sol.localdomain>

On Wed, Jul 31, 2019 at 06:11:40PM -0700, Eric Biggers wrote:
> 
> Well, it's either
> 
> 1a. Remove the user's handle.
> 	OR 
> 1b. Remove all users' handles.  (FSCRYPT_REMOVE_KEY_FLAG_ALL_USERS)
> 
> Then
> 
> 2. If no handles remain, try to evict all inodes that use the key.
> 
> By "purge all keys" do you mean step (2)?  Note that it doesn't require root by
> itself; root is only required to remove other users' handles (1b).

No, I was talking about 1b.  I'd argue that 1a and 1b should be
different ioctl.  1b requires root, and 1a doesn't.

And 1a should just mean, "I don't need to use the encrypted files any
more".  In the PAM passphrase case, when you are just logging out, 1a
is what's needed, and success is just fine.  pam_session won't *care*
whether or not there are other users keeping the key in use.

The problem with "fscrypt lock" is that the non-privileged user sort
of wants to do REMOVE_FLAG_KEY_FLAG_FOR_ALL_USERS, but they doesn't
have the privileges to do it, and they are hoping that removing their
own key removes it the key from the system.  That to me seems to be
the fundamental disconnect.  The "fscrypt unlock" and "fscrypt lock"
commands comes from the v1 key management, and requires root.  It's
the translation to unprivileged mode where "fscrypt lock" seems to
have problems.

> > What about having "fscrypt lock" call FS_IOC_GET_ENCRYPTION_KEY_STATUS
> > and print a warning message saying, "we can't lock it because N other
> > users who have registered a key".  I'd argue fscrypt should do this
> > regardless of whether or not FS_IOC_REMOVE_ENCRYPTION_KEY returns
> > EUSERS or not.
> 
> Shouldn't "fscrypt lock" still remove the user's handle, as opposed to refuse to
> do anything, though?  So it would still need to callh
> FS_IOC_REMOVE_ENCRYPTION_KEY, and could get the status from it rather than also
> needing to call FS_IOC_GET_ENCRYPTION_KEY_STATUS.
> 
> Though, FS_IOC_GET_ENCRYPTION_KEY_STATUS would be needed if we wanted to show
> the specific count of other users.
 
So my perspective is that the ioctl's should have very clean
semantics, and errors should be consistent with how the Unix system
calls and error reporting.

If we need to make "fscrypt lock" and "fscrypt unlock" have semantics
that are more consistent with previous user interface choices, then
fscrypt can use FS_IOC_GET_ENCRYPTION_KEY_STATUS to print the warning
before it calls FS_IOC_REMOVE_ENCRYPTION_KEY --- with "fscrypt purge_keys"
calling something like FS_IOC_REMOVE_ALL_USER_ENCRYPTION_KEYS.

> > It seems to me that the EBUSY and EUSERS errors should be status bits
> > which gets returned to the user in a bitfield --- and if the key has
> > been removed, or the user's claim on the key's existence has been
> > removed, the ioctl returns success.
> > 
> > That way we don't have to deal with the semantic disconnect where some
> > errors don't actually change system state, and other errors that *do*
> > change system state (as in, the key gets removed, or the user's claim
> > on the key gets removed), but still returns than error.
> > 
> 
> Do you mean use a positive return value, or do you mean add an output field to
> the struct passed to the ioctl?

I meant adding an output field.  I see EBUSY and EUSERS as status bits
which *some* use cases might find useful.  Other use cases, such as in
the pam_keys session logout code, we won't care at *all* about those
status reporting (or error codes).  So if EBUSY and EUSERS are
returned as errors, then it adds to complexity of those programs
whichd don't care.  (And even for those that do, it's still a bit more
complex since they has to distinguish between EBUSY, EUSERS, or other
errors --- in fact, *all* programs which use
FS_IOC_REMOVE_ENCRYPTION_KEY will *have* to check for EBUSY and
ESUSERS whether they care or not.)

> Either way note that it doesn't really need to be a bitfield, since you can't
> have both statuses at the same time.  I.e. if there are still other users, we
> couldn't have even gotten to checking for in-use files.

That's actually an implementation detail, though, right?  In theory,
we could check to see if there are any in-use files, independently of
whether there are any users or not.

					- Ted

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

  reply index

Thread overview: 51+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-07-26 22:41 [PATCH v7 00/16] fscrypt: key management improvements Eric Biggers
2019-07-26 22:41 ` [PATCH v7 01/16] fs, fscrypt: move uapi definitions to new header <linux/fscrypt.h> Eric Biggers
2019-07-28 15:08   ` Theodore Y. Ts'o
2019-07-26 22:41 ` [PATCH v7 02/16] fscrypt: use FSCRYPT_ prefix for uapi constants Eric Biggers
2019-07-26 22:41 ` [PATCH v7 03/16] fscrypt: use FSCRYPT_* definitions, not FS_* Eric Biggers
2019-07-26 22:41 ` [PATCH v7 04/16] fscrypt: add ->ci_inode to fscrypt_info Eric Biggers
2019-07-28 15:09   ` Theodore Y. Ts'o
2019-07-26 22:41 ` [PATCH v7 05/16] fscrypt: refactor v1 policy key setup into keysetup_legacy.c Eric Biggers
2019-07-28 15:40   ` Theodore Y. Ts'o
2019-07-29 19:37     ` Eric Biggers
2019-07-26 22:41 ` [PATCH v7 06/16] fscrypt: add FS_IOC_ADD_ENCRYPTION_KEY ioctl Eric Biggers
2019-07-28 18:50   ` Theodore Y. Ts'o
2019-07-29 19:46     ` Eric Biggers
2019-07-29 20:14       ` Theodore Y. Ts'o
2019-07-26 22:41 ` [PATCH v7 07/16] fscrypt: add FS_IOC_REMOVE_ENCRYPTION_KEY ioctl Eric Biggers
2019-07-28 19:24   ` Theodore Y. Ts'o
2019-07-29 19:58     ` Eric Biggers
2019-07-31 18:38       ` Eric Biggers
2019-07-31 23:38         ` Theodore Y. Ts'o
2019-08-01  1:11           ` [f2fs-dev] " Eric Biggers
2019-08-01  5:31             ` Theodore Y. Ts'o [this message]
2019-08-01 18:35               ` Eric Biggers
2019-08-01 18:46                 ` Eric Biggers
2019-08-01 22:04               ` Eric Biggers
2019-08-02  4:38                 ` Eric Biggers
2019-08-12 14:16                   ` Theodore Y. Ts'o
2019-07-26 22:41 ` [PATCH v7 08/16] fscrypt: add FS_IOC_GET_ENCRYPTION_KEY_STATUS ioctl Eric Biggers
2019-07-28 19:30   ` Theodore Y. Ts'o
2019-07-26 22:41 ` [PATCH v7 09/16] fscrypt: add an HKDF-SHA512 implementation Eric Biggers
2019-07-28 19:39   ` Theodore Y. Ts'o
2019-07-29 20:29     ` Eric Biggers
2019-07-29 21:42       ` James Bottomley
2019-07-26 22:41 ` [PATCH v7 10/16] fscrypt: v2 encryption policy support Eric Biggers
2019-07-28 21:17   ` Theodore Y. Ts'o
2019-07-29 20:46     ` Eric Biggers
2019-07-26 22:41 ` [PATCH v7 11/16] fscrypt: allow unprivileged users to add/remove keys for v2 policies Eric Biggers
2019-07-28 21:22   ` Theodore Y. Ts'o
2019-07-26 22:41 ` [PATCH v7 12/16] fscrypt: require that key be added when setting a v2 encryption policy Eric Biggers
2019-07-28 21:24   ` Theodore Y. Ts'o
2019-07-26 22:41 ` [PATCH v7 13/16] ext4: wire up new fscrypt ioctls Eric Biggers
2019-07-28 21:24   ` Theodore Y. Ts'o
2019-07-26 22:41 ` [PATCH v7 14/16] f2fs: " Eric Biggers
2019-07-30  0:36   ` Jaegeuk Kim
2019-08-02  8:10   ` Chao Yu
2019-08-02 17:31     ` Eric Biggers
2019-08-04  9:42       ` [f2fs-dev] " Chao Yu
2019-07-26 22:41 ` [PATCH v7 15/16] ubifs: " Eric Biggers
2019-07-30  0:39   ` Theodore Y. Ts'o
2019-07-26 22:41 ` [PATCH v7 16/16] fscrypt: document the new ioctls and policy version Eric Biggers
2019-07-29  2:00   ` Theodore Y. Ts'o
2019-07-29 21:36     ` Eric Biggers

Reply instructions:

You may reply publically 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=20190801053108.GD2769@mit.edu \
    --to=tytso@mit.edu \
    --cc=keyrings@vger.kernel.org \
    --cc=linux-api@vger.kernel.org \
    --cc=linux-crypto@vger.kernel.org \
    --cc=linux-ext4@vger.kernel.org \
    --cc=linux-f2fs-devel@lists.sourceforge.net \
    --cc=linux-fscrypt@vger.kernel.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-mtd@lists.infradead.org \
    --cc=paulcrowley@google.com \
    --cc=satyat@google.com \
    /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

Linux-mtd Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-mtd/0 linux-mtd/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-mtd linux-mtd/ https://lore.kernel.org/linux-mtd \
		linux-mtd@lists.infradead.org linux-mtd@archiver.kernel.org
	public-inbox-index linux-mtd

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-mtd


AGPL code for this site: git clone https://public-inbox.org/ public-inbox