[PATCH 0/2] avoid prepare_creds in faccessat when possible

[PATCH 0/2] avoid prepare_creds in faccessat when possible

[Mateusz Guzik]

Sometimes faccessat needs to modify current thread's credentials, but
calls prepare_creds unconditionally.

However, typically resulting credentials are identical to original ones
and in that case newcredentials are unnecessary. We can detect this before
allocating anything.

This patch series adds a helper which allows comparing capability sets and
modifies faccessat to use it.

Mateusz Guzik (2):
  CAPABILITIES: add cap_isequal helper
  fs: avoid unnecessary prepare_creds in faccessat

 fs/open.c                  | 53 ++++++++++++++++++++++++++++++----------------
 include/linux/capability.h | 10 +++++++++
 2 files changed, 45 insertions(+), 18 deletions(-)

-- 
1.8.3.1

[PATCH 1/2] CAPABILITIES: add cap_isequal helper

[Mateusz Guzik]

Can be used to determine whether two given sets have the same
capabilities.

Signed-off-by: Mateusz Guzik <mguzik@redhat.com>
---
 include/linux/capability.h | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/include/linux/capability.h b/include/linux/capability.h
index af9f0b9..2fcf941 100644
--- a/include/linux/capability.h
+++ b/include/linux/capability.h
@@ -155,6 +155,16 @@ static inline int cap_isclear(const kernel_cap_t a)
 	return 1;
 }
 
+static inline int cap_isequal(const kernel_cap_t a, const kernel_cap_t b)
+{
+	unsigned __capi;
+	CAP_FOR_EACH_U32(__capi) {
+		if (a.cap[__capi] != b.cap[__capi])
+			return 0;
+	}
+	return 1;
+}
+
 /*
  * Check if "a" is a subset of "set".
  * return 1 if ALL of the capabilities in "a" are also in "set"
-- 
1.8.3.1

[PATCH 2/2] fs: avoid unnecessary prepare_creds in faccessat

[Mateusz Guzik]

Sometimes faccessat needs to modify current thread's credentials, but
calls prepare_creds unconditionally.

Take advantage of the fact that we can detect whether any modification
to credentials is needed and in turn avoid unnecessary allocations.

Signed-off-by: Mateusz Guzik <mguzik@redhat.com>
---
 fs/open.c | 53 +++++++++++++++++++++++++++++++++++------------------
 1 file changed, 35 insertions(+), 18 deletions(-)

diff --git a/fs/open.c b/fs/open.c
index 33f9cbf..166eb45 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -330,8 +330,10 @@ SYSCALL_DEFINE4(fallocate, int, fd, int, mode, loff_t, offset, loff_t, len)
  */
 SYSCALL_DEFINE3(faccessat, int, dfd, const char __user *, filename, int, mode)
 {
-	const struct cred *old_cred;
-	struct cred *override_cred;
+	const struct cred *old_cred = current_cred();
+	struct cred *override_cred = NULL;
+	kernel_cap_t cap_effective;
+	int modify_cap_effective = 0;
 	struct path path;
 	struct inode *inode;
 	int res;
@@ -340,24 +342,37 @@ SYSCALL_DEFINE3(faccessat, int, dfd, const char __user *, filename, int, mode)
 	if (mode & ~S_IRWXO)	/* where's F_OK, X_OK, W_OK, R_OK? */
 		return -EINVAL;
 
-	override_cred = prepare_creds();
-	if (!override_cred)
-		return -ENOMEM;
-
-	override_cred->fsuid = override_cred->uid;
-	override_cred->fsgid = override_cred->gid;
-
 	if (!issecure(SECURE_NO_SETUID_FIXUP)) {
 		/* Clear the capabilities if we switch to a non-root user */
-		kuid_t root_uid = make_kuid(override_cred->user_ns, 0);
-		if (!uid_eq(override_cred->uid, root_uid))
-			cap_clear(override_cred->cap_effective);
-		else
-			override_cred->cap_effective =
-				override_cred->cap_permitted;
+		kuid_t root_uid = make_kuid(old_cred->user_ns, 0);
+		if (!uid_eq(old_cred->uid, root_uid)) {
+			if (!cap_isclear(old_cred->cap_effective)) {
+				cap_clear(cap_effective);
+				modify_cap_effective = 1;
+			}
+		} else {
+			if (!cap_isequal(old_cred->cap_effective,
+				old_cred->cap_permitted)) {
+				cap_effective = old_cred->cap_permitted;
+				modify_cap_effective = 1;
+			}
+		}
 	}
 
-	old_cred = override_creds(override_cred);
+	if (!uid_eq(old_cred->fsuid, old_cred->uid) ||
+	    !gid_eq(old_cred->fsgid, old_cred->gid) ||
+	    modify_cap_effective) {
+		override_cred = prepare_creds();
+		if (!override_cred)
+			return -ENOMEM;
+
+		override_cred->fsuid = override_cred->uid;
+		override_cred->fsgid = override_cred->gid;
+		if (modify_cap_effective)
+			override_cred->cap_effective = cap_effective;
+
+		override_creds(override_cred);
+	}
 retry:
 	res = user_path_at(dfd, filename, lookup_flags, &path);
 	if (res)
@@ -399,8 +414,10 @@ out_path_release:
 		goto retry;
 	}
 out:
-	revert_creds(old_cred);
-	put_cred(override_cred);
+	if (override_cred) {
+		revert_creds(old_cred);
+		put_cred(override_cred);
+	}
 	return res;
 }
 
-- 
1.8.3.1

Re: [PATCH 0/2] avoid prepare_creds in faccessat when possible

[Mateusz Guzik]

On Mon, Mar 09, 2015 at 09:35:45PM +0100, Mateusz Guzik wrote:
> Sometimes faccessat needs to modify current thread's credentials, but
> calls prepare_creds unconditionally.
> 
> However, typically resulting credentials are identical to original ones
> and in that case newcredentials are unnecessary. We can detect this before
> allocating anything.
> 
> This patch series adds a helper which allows comparing capability sets and
> modifies faccessat to use it.
> 
> Mateusz Guzik (2):
>   CAPABILITIES: add cap_isequal helper
>   fs: avoid unnecessary prepare_creds in faccessat
> 

Can I get some (N)ACKs on this one?

Thanks,
-- 
Mateusz Guzik

Re: [PATCH 1/2] CAPABILITIES: add cap_isequal helper

[Paul Moore]

On Monday, March 09, 2015 09:35:46 PM Mateusz Guzik wrote:
> Can be used to determine whether two given sets have the same
> capabilities.
> 
> Signed-off-by: Mateusz Guzik <mguzik@redhat.com>
> ---
>  include/linux/capability.h | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/include/linux/capability.h b/include/linux/capability.h
> index af9f0b9..2fcf941 100644
> --- a/include/linux/capability.h
> +++ b/include/linux/capability.h
> @@ -155,6 +155,16 @@ static inline int cap_isclear(const kernel_cap_t a)
>  	return 1;
>  }
> 
> +static inline int cap_isequal(const kernel_cap_t a, const kernel_cap_t b)
> +{
> +	unsigned __capi;
> +	CAP_FOR_EACH_U32(__capi) {
> +		if (a.cap[__capi] != b.cap[__capi])
> +			return 0;
> +	}
> +	return 1;
> +}

I realize it is currently only a two pass loop so probably not that big of a 
deal, but couldn't you accomplish the same with a memcmp()?  I suppose the 
above implementation might be faster than those architectures which use the 
generic memcmp() implementation, but I wonder if the arch-specific memcmp() 
implementations would be faster.

Also, what is the main motivation for this patchset?  Do you have a workload 
that is being hit hard by prepare_creds()?

-- 
paul moore
security @ redhat

Re: [PATCH 1/2] CAPABILITIES: add cap_isequal helper

[Mateusz Guzik]

On Fri, Mar 13, 2015 at 10:02:46AM -0400, Paul Moore wrote:
> On Monday, March 09, 2015 09:35:46 PM Mateusz Guzik wrote:
> > Can be used to determine whether two given sets have the same
> > capabilities.
> > 
> > Signed-off-by: Mateusz Guzik <mguzik@redhat.com>
> > ---
> >  include/linux/capability.h | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> > 
> > diff --git a/include/linux/capability.h b/include/linux/capability.h
> > index af9f0b9..2fcf941 100644
> > --- a/include/linux/capability.h
> > +++ b/include/linux/capability.h
> > @@ -155,6 +155,16 @@ static inline int cap_isclear(const kernel_cap_t a)
> >  	return 1;
> >  }
> > 
> > +static inline int cap_isequal(const kernel_cap_t a, const kernel_cap_t b)
> > +{
> > +	unsigned __capi;
> > +	CAP_FOR_EACH_U32(__capi) {
> > +		if (a.cap[__capi] != b.cap[__capi])
> > +			return 0;
> > +	}
> > +	return 1;
> > +}
> 
> I realize it is currently only a two pass loop so probably not that big of a 
> deal, but couldn't you accomplish the same with a memcmp()?  I suppose the 
> above implementation might be faster than those architectures which use the 
> generic memcmp() implementation, but I wonder if the arch-specific memcmp() 
> implementations would be faster.
> 

Well I did it this way for consistency with the rest of the file. Trying
to use memcpy with only 2 elements to compare may be a dubious
optimisation and would require providing additional macros for cap size.

As such, I would prefer to keep the loop as it is. This can be changed
should caps ever grow.

> Also, what is the main motivation for this patchset?  Do you have a workload 
> that is being hit hard by prepare_creds()?
> 

It's just something I stumbled upon and decided to microoptimize (fwiw,
faccessat is called quite often, but not enough for this change to be
world-changing).

Given the triviality of the patch I figured it should be fine to do it.

-- 
Mateusz Guzik