All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] cred: Do not default to init_cred in prepare_kernel_cred()
@ 2022-10-26 23:31 Kees Cook
  2022-10-27  6:49 ` Greg Kroah-Hartman
                   ` (4 more replies)
  0 siblings, 5 replies; 6+ messages in thread
From: Kees Cook @ 2022-10-26 23:31 UTC (permalink / raw)
  To: Eric W. Biederman
  Cc: Kees Cook, David Howells, Luis Chamberlain, Russ Weight,
	Greg Kroah-Hartman, Rafael J. Wysocki, Steve French,
	Paulo Alcantara, Ronnie Sahlberg, Shyam Prasad N, Tom Talpey,
	Namjae Jeon, Sergey Senozhatsky, Trond Myklebust, Anna Schumaker,
	Chuck Lever, Jeff Layton, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Michal Koutný,
	Peter Zijlstra, linux-cifs, samba-technical, linux-nfs, netdev,
	linux-kernel, linux-hardening

A common exploit pattern for ROP attacks is to abuse prepare_kernel_cred()
in order to construct escalated privileges[1]. Instead of providing a
short-hand argument (NULL) to the "daemon" argument to indicate using
init_cred as the base cred, require that "daemon" is always set to
an actual task. Replace all existing callers that were passing NULL
with &init_task.

Future attacks will need to have sufficiently powerful read/write
primitives to have found an appropriately privileged task and written it
to the ROP stack as an argument to succeed, which is similarly difficult
to the prior effort needed to escalate privileges before struct cred
existed: locate the current cred and overwrite the uid member.

This has the added benefit of meaning that prepare_kernel_cred() can no
longer exceed the privileges of the init task, which may have changed from
the original init_cred (e.g. dropping capabilities from the bounding set).

[1] https://google.com/search?q=commit_creds(prepare_kernel_cred(0))

Cc: "Eric W. Biederman" <ebiederm@xmission.com>
Cc: David Howells <dhowells@redhat.com>
Cc: Luis Chamberlain <mcgrof@kernel.org>
Cc: Russ Weight <russell.h.weight@intel.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: Steve French <sfrench@samba.org>
Cc: Paulo Alcantara <pc@cjr.nz>
Cc: Ronnie Sahlberg <lsahlber@redhat.com>
Cc: Shyam Prasad N <sprasad@microsoft.com>
Cc: Tom Talpey <tom@talpey.com>
Cc: Namjae Jeon <linkinjeon@kernel.org>
Cc: Sergey Senozhatsky <senozhatsky@chromium.org>
Cc: Trond Myklebust <trond.myklebust@hammerspace.com>
Cc: Anna Schumaker <anna@kernel.org>
Cc: Chuck Lever <chuck.lever@oracle.com>
Cc: Jeff Layton <jlayton@kernel.org>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Paolo Abeni <pabeni@redhat.com>
Cc: "Michal Koutný" <mkoutny@suse.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: linux-cifs@vger.kernel.org
Cc: samba-technical@lists.samba.org
Cc: linux-nfs@vger.kernel.org
Cc: netdev@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 drivers/base/firmware_loader/main.c    |  2 +-
 fs/cifs/cifs_spnego.c                  |  2 +-
 fs/cifs/cifsacl.c                      |  2 +-
 fs/ksmbd/smb_common.c                  |  2 +-
 fs/nfs/flexfilelayout/flexfilelayout.c |  4 ++--
 fs/nfs/nfs4idmap.c                     |  2 +-
 fs/nfsd/nfs4callback.c                 |  2 +-
 kernel/cred.c                          | 15 +++++++--------
 net/dns_resolver/dns_key.c             |  2 +-
 9 files changed, 16 insertions(+), 17 deletions(-)

diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c
index 7c3590fd97c2..017c4cdb219e 100644
--- a/drivers/base/firmware_loader/main.c
+++ b/drivers/base/firmware_loader/main.c
@@ -821,7 +821,7 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
 	 * called by a driver when serving an unrelated request from userland, we use
 	 * the kernel credentials to read the file.
 	 */
-	kern_cred = prepare_kernel_cred(NULL);
+	kern_cred = prepare_kernel_cred(&init_task);
 	if (!kern_cred) {
 		ret = -ENOMEM;
 		goto out;
diff --git a/fs/cifs/cifs_spnego.c b/fs/cifs/cifs_spnego.c
index 342717bf1dc2..6f3285f1dfee 100644
--- a/fs/cifs/cifs_spnego.c
+++ b/fs/cifs/cifs_spnego.c
@@ -189,7 +189,7 @@ init_cifs_spnego(void)
 	 * spnego upcalls.
 	 */
 
-	cred = prepare_kernel_cred(NULL);
+	cred = prepare_kernel_cred(&init_task);
 	if (!cred)
 		return -ENOMEM;
 
diff --git a/fs/cifs/cifsacl.c b/fs/cifs/cifsacl.c
index fa480d62f313..574de2b225ae 100644
--- a/fs/cifs/cifsacl.c
+++ b/fs/cifs/cifsacl.c
@@ -465,7 +465,7 @@ init_cifs_idmap(void)
 	 * this is used to prevent malicious redirections from being installed
 	 * with add_key().
 	 */
-	cred = prepare_kernel_cred(NULL);
+	cred = prepare_kernel_cred(&init_task);
 	if (!cred)
 		return -ENOMEM;
 
diff --git a/fs/ksmbd/smb_common.c b/fs/ksmbd/smb_common.c
index d96da872d70a..2a4fbbd55b91 100644
--- a/fs/ksmbd/smb_common.c
+++ b/fs/ksmbd/smb_common.c
@@ -623,7 +623,7 @@ int ksmbd_override_fsids(struct ksmbd_work *work)
 	if (share->force_gid != KSMBD_SHARE_INVALID_GID)
 		gid = share->force_gid;
 
-	cred = prepare_kernel_cred(NULL);
+	cred = prepare_kernel_cred(&init_task);
 	if (!cred)
 		return -ENOMEM;
 
diff --git a/fs/nfs/flexfilelayout/flexfilelayout.c b/fs/nfs/flexfilelayout/flexfilelayout.c
index 1ec79ccf89ad..7deb3cd76abe 100644
--- a/fs/nfs/flexfilelayout/flexfilelayout.c
+++ b/fs/nfs/flexfilelayout/flexfilelayout.c
@@ -493,10 +493,10 @@ ff_layout_alloc_lseg(struct pnfs_layout_hdr *lh,
 		gid = make_kgid(&init_user_ns, id);
 
 		if (gfp_flags & __GFP_FS)
-			kcred = prepare_kernel_cred(NULL);
+			kcred = prepare_kernel_cred(&init_task);
 		else {
 			unsigned int nofs_flags = memalloc_nofs_save();
-			kcred = prepare_kernel_cred(NULL);
+			kcred = prepare_kernel_cred(&init_task);
 			memalloc_nofs_restore(nofs_flags);
 		}
 		rc = -ENOMEM;
diff --git a/fs/nfs/nfs4idmap.c b/fs/nfs/nfs4idmap.c
index e3fdd2f45b01..25a7c771cfd8 100644
--- a/fs/nfs/nfs4idmap.c
+++ b/fs/nfs/nfs4idmap.c
@@ -203,7 +203,7 @@ int nfs_idmap_init(void)
 	printk(KERN_NOTICE "NFS: Registering the %s key type\n",
 		key_type_id_resolver.name);
 
-	cred = prepare_kernel_cred(NULL);
+	cred = prepare_kernel_cred(&init_task);
 	if (!cred)
 		return -ENOMEM;
 
diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
index f0e69edf5f0f..4a9e8d17e56a 100644
--- a/fs/nfsd/nfs4callback.c
+++ b/fs/nfsd/nfs4callback.c
@@ -870,7 +870,7 @@ static const struct cred *get_backchannel_cred(struct nfs4_client *clp, struct r
 	} else {
 		struct cred *kcred;
 
-		kcred = prepare_kernel_cred(NULL);
+		kcred = prepare_kernel_cred(&init_task);
 		if (!kcred)
 			return NULL;
 
diff --git a/kernel/cred.c b/kernel/cred.c
index e10c15f51c1f..811ad654abd1 100644
--- a/kernel/cred.c
+++ b/kernel/cred.c
@@ -701,9 +701,9 @@ void __init cred_init(void)
  * override a task's own credentials so that work can be done on behalf of that
  * task that requires a different subjective context.
  *
- * @daemon is used to provide a base for the security record, but can be NULL.
- * If @daemon is supplied, then the security data will be derived from that;
- * otherwise they'll be set to 0 and no groups, full capabilities and no keys.
+ * @daemon is used to provide a base cred, with the security data derived from
+ * that; if this is "&init_task", they'll be set to 0, no groups, full
+ * capabilities, and no keys.
  *
  * The caller may change these controls afterwards if desired.
  *
@@ -714,17 +714,16 @@ struct cred *prepare_kernel_cred(struct task_struct *daemon)
 	const struct cred *old;
 	struct cred *new;
 
+	if (WARN_ON_ONCE(!daemon))
+		return NULL;
+
 	new = kmem_cache_alloc(cred_jar, GFP_KERNEL);
 	if (!new)
 		return NULL;
 
 	kdebug("prepare_kernel_cred() alloc %p", new);
 
-	if (daemon)
-		old = get_task_cred(daemon);
-	else
-		old = get_cred(&init_cred);
-
+	old = get_task_cred(daemon);
 	validate_creds(old);
 
 	*new = *old;
diff --git a/net/dns_resolver/dns_key.c b/net/dns_resolver/dns_key.c
index 3aced951d5ab..01e54b46ae0b 100644
--- a/net/dns_resolver/dns_key.c
+++ b/net/dns_resolver/dns_key.c
@@ -337,7 +337,7 @@ static int __init init_dns_resolver(void)
 	 * this is used to prevent malicious redirections from being installed
 	 * with add_key().
 	 */
-	cred = prepare_kernel_cred(NULL);
+	cred = prepare_kernel_cred(&init_task);
 	if (!cred)
 		return -ENOMEM;
 
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH] cred: Do not default to init_cred in prepare_kernel_cred()
  2022-10-26 23:31 [PATCH] cred: Do not default to init_cred in prepare_kernel_cred() Kees Cook
@ 2022-10-27  6:49 ` Greg Kroah-Hartman
  2022-10-27 12:03 ` Paulo Alcantara
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 6+ messages in thread
From: Greg Kroah-Hartman @ 2022-10-27  6:49 UTC (permalink / raw)
  To: Kees Cook
  Cc: Eric W. Biederman, David Howells, Luis Chamberlain, Russ Weight,
	Rafael J. Wysocki, Steve French, Paulo Alcantara,
	Ronnie Sahlberg, Shyam Prasad N, Tom Talpey, Namjae Jeon,
	Sergey Senozhatsky, Trond Myklebust, Anna Schumaker, Chuck Lever,
	Jeff Layton, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Michal Koutný,
	Peter Zijlstra, linux-cifs, samba-technical, linux-nfs, netdev,
	linux-kernel, linux-hardening

On Wed, Oct 26, 2022 at 04:31:11PM -0700, Kees Cook wrote:
> A common exploit pattern for ROP attacks is to abuse prepare_kernel_cred()
> in order to construct escalated privileges[1]. Instead of providing a
> short-hand argument (NULL) to the "daemon" argument to indicate using
> init_cred as the base cred, require that "daemon" is always set to
> an actual task. Replace all existing callers that were passing NULL
> with &init_task.
> 
> Future attacks will need to have sufficiently powerful read/write
> primitives to have found an appropriately privileged task and written it
> to the ROP stack as an argument to succeed, which is similarly difficult
> to the prior effort needed to escalate privileges before struct cred
> existed: locate the current cred and overwrite the uid member.
> 
> This has the added benefit of meaning that prepare_kernel_cred() can no
> longer exceed the privileges of the init task, which may have changed from
> the original init_cred (e.g. dropping capabilities from the bounding set).
> 
> [1] https://google.com/search?q=commit_creds(prepare_kernel_cred(0))
> 
> Cc: "Eric W. Biederman" <ebiederm@xmission.com>
> Cc: David Howells <dhowells@redhat.com>
> Cc: Luis Chamberlain <mcgrof@kernel.org>
> Cc: Russ Weight <russell.h.weight@intel.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> Cc: Steve French <sfrench@samba.org>
> Cc: Paulo Alcantara <pc@cjr.nz>
> Cc: Ronnie Sahlberg <lsahlber@redhat.com>
> Cc: Shyam Prasad N <sprasad@microsoft.com>
> Cc: Tom Talpey <tom@talpey.com>
> Cc: Namjae Jeon <linkinjeon@kernel.org>
> Cc: Sergey Senozhatsky <senozhatsky@chromium.org>
> Cc: Trond Myklebust <trond.myklebust@hammerspace.com>
> Cc: Anna Schumaker <anna@kernel.org>
> Cc: Chuck Lever <chuck.lever@oracle.com>
> Cc: Jeff Layton <jlayton@kernel.org>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Eric Dumazet <edumazet@google.com>
> Cc: Jakub Kicinski <kuba@kernel.org>
> Cc: Paolo Abeni <pabeni@redhat.com>
> Cc: "Michal Koutný" <mkoutny@suse.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: linux-cifs@vger.kernel.org
> Cc: samba-technical@lists.samba.org
> Cc: linux-nfs@vger.kernel.org
> Cc: netdev@vger.kernel.org
> Signed-off-by: Kees Cook <keescook@chromium.org>

Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] cred: Do not default to init_cred in prepare_kernel_cred()
  2022-10-26 23:31 [PATCH] cred: Do not default to init_cred in prepare_kernel_cred() Kees Cook
  2022-10-27  6:49 ` Greg Kroah-Hartman
@ 2022-10-27 12:03 ` Paulo Alcantara
  2022-10-27 13:07 ` Sergey Senozhatsky
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 6+ messages in thread
From: Paulo Alcantara @ 2022-10-27 12:03 UTC (permalink / raw)
  To: Kees Cook, Eric W. Biederman
  Cc: Kees Cook, David Howells, Luis Chamberlain, Russ Weight,
	Greg Kroah-Hartman, Rafael J. Wysocki, Steve French,
	Ronnie Sahlberg, Shyam Prasad N, Tom Talpey, Namjae Jeon,
	Sergey Senozhatsky, Trond Myklebust, Anna Schumaker, Chuck Lever,
	Jeff Layton, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Michal Koutný,
	Peter Zijlstra, linux-cifs, samba-technical, linux-nfs, netdev,
	linux-kernel, linux-hardening

Kees Cook <keescook@chromium.org> writes:

> A common exploit pattern for ROP attacks is to abuse prepare_kernel_cred()
> in order to construct escalated privileges[1]. Instead of providing a
> short-hand argument (NULL) to the "daemon" argument to indicate using
> init_cred as the base cred, require that "daemon" is always set to
> an actual task. Replace all existing callers that were passing NULL
> with &init_task.
>
> Future attacks will need to have sufficiently powerful read/write
> primitives to have found an appropriately privileged task and written it
> to the ROP stack as an argument to succeed, which is similarly difficult
> to the prior effort needed to escalate privileges before struct cred
> existed: locate the current cred and overwrite the uid member.
>
> This has the added benefit of meaning that prepare_kernel_cred() can no
> longer exceed the privileges of the init task, which may have changed from
> the original init_cred (e.g. dropping capabilities from the bounding set).
>
> [1] https://google.com/search?q=commit_creds(prepare_kernel_cred(0))
>
> Cc: "Eric W. Biederman" <ebiederm@xmission.com>
> Cc: David Howells <dhowells@redhat.com>
> Cc: Luis Chamberlain <mcgrof@kernel.org>
> Cc: Russ Weight <russell.h.weight@intel.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> Cc: Steve French <sfrench@samba.org>
> Cc: Paulo Alcantara <pc@cjr.nz>
> Cc: Ronnie Sahlberg <lsahlber@redhat.com>
> Cc: Shyam Prasad N <sprasad@microsoft.com>
> Cc: Tom Talpey <tom@talpey.com>
> Cc: Namjae Jeon <linkinjeon@kernel.org>
> Cc: Sergey Senozhatsky <senozhatsky@chromium.org>
> Cc: Trond Myklebust <trond.myklebust@hammerspace.com>
> Cc: Anna Schumaker <anna@kernel.org>
> Cc: Chuck Lever <chuck.lever@oracle.com>
> Cc: Jeff Layton <jlayton@kernel.org>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Eric Dumazet <edumazet@google.com>
> Cc: Jakub Kicinski <kuba@kernel.org>
> Cc: Paolo Abeni <pabeni@redhat.com>
> Cc: "Michal Koutný" <mkoutny@suse.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: linux-cifs@vger.kernel.org
> Cc: samba-technical@lists.samba.org
> Cc: linux-nfs@vger.kernel.org
> Cc: netdev@vger.kernel.org
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  drivers/base/firmware_loader/main.c    |  2 +-
>  fs/cifs/cifs_spnego.c                  |  2 +-
>  fs/cifs/cifsacl.c                      |  2 +-
>  fs/ksmbd/smb_common.c                  |  2 +-
>  fs/nfs/flexfilelayout/flexfilelayout.c |  4 ++--
>  fs/nfs/nfs4idmap.c                     |  2 +-
>  fs/nfsd/nfs4callback.c                 |  2 +-
>  kernel/cred.c                          | 15 +++++++--------
>  net/dns_resolver/dns_key.c             |  2 +-
>  9 files changed, 16 insertions(+), 17 deletions(-)

Acked-by: Paulo Alcantara (SUSE) <pc@cjr.nz>

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] cred: Do not default to init_cred in prepare_kernel_cred()
  2022-10-26 23:31 [PATCH] cred: Do not default to init_cred in prepare_kernel_cred() Kees Cook
  2022-10-27  6:49 ` Greg Kroah-Hartman
  2022-10-27 12:03 ` Paulo Alcantara
@ 2022-10-27 13:07 ` Sergey Senozhatsky
  2022-10-27 17:03 ` Luis Chamberlain
  2022-10-27 17:16 ` Russ Weight
  4 siblings, 0 replies; 6+ messages in thread
From: Sergey Senozhatsky @ 2022-10-27 13:07 UTC (permalink / raw)
  To: Kees Cook
  Cc: Eric W. Biederman, David Howells, Luis Chamberlain, Russ Weight,
	Greg Kroah-Hartman, Rafael J. Wysocki, Steve French,
	Paulo Alcantara, Ronnie Sahlberg, Shyam Prasad N, Tom Talpey,
	Namjae Jeon, Sergey Senozhatsky, Trond Myklebust, Anna Schumaker,
	Chuck Lever, Jeff Layton, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Michal Koutný,
	Peter Zijlstra, linux-cifs, samba-technical, linux-nfs, netdev,
	linux-kernel, linux-hardening

On (22/10/26 16:31), Kees Cook wrote:
> A common exploit pattern for ROP attacks is to abuse prepare_kernel_cred()
> in order to construct escalated privileges[1]. Instead of providing a
> short-hand argument (NULL) to the "daemon" argument to indicate using
> init_cred as the base cred, require that "daemon" is always set to
> an actual task. Replace all existing callers that were passing NULL
> with &init_task.
> 
> Future attacks will need to have sufficiently powerful read/write
> primitives to have found an appropriately privileged task and written it
> to the ROP stack as an argument to succeed, which is similarly difficult
> to the prior effort needed to escalate privileges before struct cred
> existed: locate the current cred and overwrite the uid member.
> 
> This has the added benefit of meaning that prepare_kernel_cred() can no
> longer exceed the privileges of the init task, which may have changed from
> the original init_cred (e.g. dropping capabilities from the bounding set).

Reviewed-by: Sergey Senozhatsky <senozhatsky@chromium.org>

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] cred: Do not default to init_cred in prepare_kernel_cred()
  2022-10-26 23:31 [PATCH] cred: Do not default to init_cred in prepare_kernel_cred() Kees Cook
                   ` (2 preceding siblings ...)
  2022-10-27 13:07 ` Sergey Senozhatsky
@ 2022-10-27 17:03 ` Luis Chamberlain
  2022-10-27 17:16 ` Russ Weight
  4 siblings, 0 replies; 6+ messages in thread
From: Luis Chamberlain @ 2022-10-27 17:03 UTC (permalink / raw)
  To: Kees Cook
  Cc: Eric W. Biederman, David Howells, Russ Weight,
	Greg Kroah-Hartman, Rafael J. Wysocki, Steve French,
	Paulo Alcantara, Ronnie Sahlberg, Shyam Prasad N, Tom Talpey,
	Namjae Jeon, Sergey Senozhatsky, Trond Myklebust, Anna Schumaker,
	Chuck Lever, Jeff Layton, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Michal Koutný,
	Peter Zijlstra, linux-cifs, samba-technical, linux-nfs, netdev,
	linux-kernel, linux-hardening

On Wed, Oct 26, 2022 at 04:31:11PM -0700, Kees Cook wrote:
> A common exploit pattern for ROP attacks is to abuse prepare_kernel_cred()
> in order to construct escalated privileges[1]. Instead of providing a
> short-hand argument (NULL) to the "daemon" argument to indicate using
> init_cred as the base cred, require that "daemon" is always set to
> an actual task. Replace all existing callers that were passing NULL
> with &init_task.
> 
> Future attacks will need to have sufficiently powerful read/write
> primitives to have found an appropriately privileged task and written it
> to the ROP stack as an argument to succeed, which is similarly difficult
> to the prior effort needed to escalate privileges before struct cred
> existed: locate the current cred and overwrite the uid member.
> 
> This has the added benefit of meaning that prepare_kernel_cred() can no
> longer exceed the privileges of the init task, which may have changed from
> the original init_cred (e.g. dropping capabilities from the bounding set).
> 
> [1] https://google.com/search?q=commit_creds(prepare_kernel_cred(0))
> 
> Cc: "Eric W. Biederman" <ebiederm@xmission.com>
> Cc: David Howells <dhowells@redhat.com>
> Cc: Luis Chamberlain <mcgrof@kernel.org>
> Cc: Russ Weight <russell.h.weight@intel.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> Cc: Steve French <sfrench@samba.org>
> Cc: Paulo Alcantara <pc@cjr.nz>
> Cc: Ronnie Sahlberg <lsahlber@redhat.com>
> Cc: Shyam Prasad N <sprasad@microsoft.com>
> Cc: Tom Talpey <tom@talpey.com>
> Cc: Namjae Jeon <linkinjeon@kernel.org>
> Cc: Sergey Senozhatsky <senozhatsky@chromium.org>
> Cc: Trond Myklebust <trond.myklebust@hammerspace.com>
> Cc: Anna Schumaker <anna@kernel.org>
> Cc: Chuck Lever <chuck.lever@oracle.com>
> Cc: Jeff Layton <jlayton@kernel.org>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Eric Dumazet <edumazet@google.com>
> Cc: Jakub Kicinski <kuba@kernel.org>
> Cc: Paolo Abeni <pabeni@redhat.com>
> Cc: "Michal Koutný" <mkoutny@suse.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: linux-cifs@vger.kernel.org
> Cc: samba-technical@lists.samba.org
> Cc: linux-nfs@vger.kernel.org
> Cc: netdev@vger.kernel.org
> Signed-off-by: Kees Cook <keescook@chromium.org>

Acked-by: Luis Chamberlain <mcgrof@kernel.org>

  Luis

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] cred: Do not default to init_cred in prepare_kernel_cred()
  2022-10-26 23:31 [PATCH] cred: Do not default to init_cred in prepare_kernel_cred() Kees Cook
                   ` (3 preceding siblings ...)
  2022-10-27 17:03 ` Luis Chamberlain
@ 2022-10-27 17:16 ` Russ Weight
  4 siblings, 0 replies; 6+ messages in thread
From: Russ Weight @ 2022-10-27 17:16 UTC (permalink / raw)
  To: Kees Cook, Eric W. Biederman
  Cc: David Howells, Luis Chamberlain, Greg Kroah-Hartman,
	Rafael J. Wysocki, Steve French, Paulo Alcantara,
	Ronnie Sahlberg, Shyam Prasad N, Tom Talpey, Namjae Jeon,
	Sergey Senozhatsky, Trond Myklebust, Anna Schumaker, Chuck Lever,
	Jeff Layton, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Michal Koutný,
	Peter Zijlstra, linux-cifs, samba-technical, linux-nfs, netdev,
	linux-kernel, linux-hardening



On 10/26/22 16:31, Kees Cook wrote:
> A common exploit pattern for ROP attacks is to abuse prepare_kernel_cred()
> in order to construct escalated privileges[1]. Instead of providing a
> short-hand argument (NULL) to the "daemon" argument to indicate using
> init_cred as the base cred, require that "daemon" is always set to
> an actual task. Replace all existing callers that were passing NULL
> with &init_task.
>
> Future attacks will need to have sufficiently powerful read/write
> primitives to have found an appropriately privileged task and written it
> to the ROP stack as an argument to succeed, which is similarly difficult
> to the prior effort needed to escalate privileges before struct cred
> existed: locate the current cred and overwrite the uid member.
>
> This has the added benefit of meaning that prepare_kernel_cred() can no
> longer exceed the privileges of the init task, which may have changed from
> the original init_cred (e.g. dropping capabilities from the bounding set).
>
> [1] https://google.com/search?q=commit_creds(prepare_kernel_cred(0))
>
> Cc: "Eric W. Biederman" <ebiederm@xmission.com>
> Cc: David Howells <dhowells@redhat.com>
> Cc: Luis Chamberlain <mcgrof@kernel.org>
> Cc: Russ Weight <russell.h.weight@intel.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> Cc: Steve French <sfrench@samba.org>
> Cc: Paulo Alcantara <pc@cjr.nz>
> Cc: Ronnie Sahlberg <lsahlber@redhat.com>
> Cc: Shyam Prasad N <sprasad@microsoft.com>
> Cc: Tom Talpey <tom@talpey.com>
> Cc: Namjae Jeon <linkinjeon@kernel.org>
> Cc: Sergey Senozhatsky <senozhatsky@chromium.org>
> Cc: Trond Myklebust <trond.myklebust@hammerspace.com>
> Cc: Anna Schumaker <anna@kernel.org>
> Cc: Chuck Lever <chuck.lever@oracle.com>
> Cc: Jeff Layton <jlayton@kernel.org>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Eric Dumazet <edumazet@google.com>
> Cc: Jakub Kicinski <kuba@kernel.org>
> Cc: Paolo Abeni <pabeni@redhat.com>
> Cc: "Michal Koutný" <mkoutny@suse.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: linux-cifs@vger.kernel.org
> Cc: samba-technical@lists.samba.org
> Cc: linux-nfs@vger.kernel.org
> Cc: netdev@vger.kernel.org
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  drivers/base/firmware_loader/main.c    |  2 +-
>  fs/cifs/cifs_spnego.c                  |  2 +-
>  fs/cifs/cifsacl.c                      |  2 +-
>  fs/ksmbd/smb_common.c                  |  2 +-
>  fs/nfs/flexfilelayout/flexfilelayout.c |  4 ++--
>  fs/nfs/nfs4idmap.c                     |  2 +-
>  fs/nfsd/nfs4callback.c                 |  2 +-
>  kernel/cred.c                          | 15 +++++++--------
>  net/dns_resolver/dns_key.c             |  2 +-
>  9 files changed, 16 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c
> index 7c3590fd97c2..017c4cdb219e 100644
> --- a/drivers/base/firmware_loader/main.c
> +++ b/drivers/base/firmware_loader/main.c
> @@ -821,7 +821,7 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
>  	 * called by a driver when serving an unrelated request from userland, we use
>  	 * the kernel credentials to read the file.
>  	 */
> -	kern_cred = prepare_kernel_cred(NULL);
> +	kern_cred = prepare_kernel_cred(&init_task);
>  	if (!kern_cred) {
>  		ret = -ENOMEM;
>  		goto out;
> diff --git a/fs/cifs/cifs_spnego.c b/fs/cifs/cifs_spnego.c
> index 342717bf1dc2..6f3285f1dfee 100644
> --- a/fs/cifs/cifs_spnego.c
> +++ b/fs/cifs/cifs_spnego.c
> @@ -189,7 +189,7 @@ init_cifs_spnego(void)
>  	 * spnego upcalls.
>  	 */
>  
> -	cred = prepare_kernel_cred(NULL);
> +	cred = prepare_kernel_cred(&init_task);
>  	if (!cred)
>  		return -ENOMEM;
>  
> diff --git a/fs/cifs/cifsacl.c b/fs/cifs/cifsacl.c
> index fa480d62f313..574de2b225ae 100644
> --- a/fs/cifs/cifsacl.c
> +++ b/fs/cifs/cifsacl.c
> @@ -465,7 +465,7 @@ init_cifs_idmap(void)
>  	 * this is used to prevent malicious redirections from being installed
>  	 * with add_key().
>  	 */
> -	cred = prepare_kernel_cred(NULL);
> +	cred = prepare_kernel_cred(&init_task);
>  	if (!cred)
>  		return -ENOMEM;
>  
> diff --git a/fs/ksmbd/smb_common.c b/fs/ksmbd/smb_common.c
> index d96da872d70a..2a4fbbd55b91 100644
> --- a/fs/ksmbd/smb_common.c
> +++ b/fs/ksmbd/smb_common.c
> @@ -623,7 +623,7 @@ int ksmbd_override_fsids(struct ksmbd_work *work)
>  	if (share->force_gid != KSMBD_SHARE_INVALID_GID)
>  		gid = share->force_gid;
>  
> -	cred = prepare_kernel_cred(NULL);
> +	cred = prepare_kernel_cred(&init_task);
>  	if (!cred)
>  		return -ENOMEM;
>  
> diff --git a/fs/nfs/flexfilelayout/flexfilelayout.c b/fs/nfs/flexfilelayout/flexfilelayout.c
> index 1ec79ccf89ad..7deb3cd76abe 100644
> --- a/fs/nfs/flexfilelayout/flexfilelayout.c
> +++ b/fs/nfs/flexfilelayout/flexfilelayout.c
> @@ -493,10 +493,10 @@ ff_layout_alloc_lseg(struct pnfs_layout_hdr *lh,
>  		gid = make_kgid(&init_user_ns, id);
>  
>  		if (gfp_flags & __GFP_FS)
> -			kcred = prepare_kernel_cred(NULL);
> +			kcred = prepare_kernel_cred(&init_task);
>  		else {
>  			unsigned int nofs_flags = memalloc_nofs_save();
> -			kcred = prepare_kernel_cred(NULL);
> +			kcred = prepare_kernel_cred(&init_task);
>  			memalloc_nofs_restore(nofs_flags);
>  		}
>  		rc = -ENOMEM;
> diff --git a/fs/nfs/nfs4idmap.c b/fs/nfs/nfs4idmap.c
> index e3fdd2f45b01..25a7c771cfd8 100644
> --- a/fs/nfs/nfs4idmap.c
> +++ b/fs/nfs/nfs4idmap.c
> @@ -203,7 +203,7 @@ int nfs_idmap_init(void)
>  	printk(KERN_NOTICE "NFS: Registering the %s key type\n",
>  		key_type_id_resolver.name);
>  
> -	cred = prepare_kernel_cred(NULL);
> +	cred = prepare_kernel_cred(&init_task);
>  	if (!cred)
>  		return -ENOMEM;
>  
> diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
> index f0e69edf5f0f..4a9e8d17e56a 100644
> --- a/fs/nfsd/nfs4callback.c
> +++ b/fs/nfsd/nfs4callback.c
> @@ -870,7 +870,7 @@ static const struct cred *get_backchannel_cred(struct nfs4_client *clp, struct r
>  	} else {
>  		struct cred *kcred;
>  
> -		kcred = prepare_kernel_cred(NULL);
> +		kcred = prepare_kernel_cred(&init_task);
>  		if (!kcred)
>  			return NULL;
>  
> diff --git a/kernel/cred.c b/kernel/cred.c
> index e10c15f51c1f..811ad654abd1 100644
> --- a/kernel/cred.c
> +++ b/kernel/cred.c
> @@ -701,9 +701,9 @@ void __init cred_init(void)
>   * override a task's own credentials so that work can be done on behalf of that
>   * task that requires a different subjective context.
>   *
> - * @daemon is used to provide a base for the security record, but can be NULL.
> - * If @daemon is supplied, then the security data will be derived from that;
> - * otherwise they'll be set to 0 and no groups, full capabilities and no keys.
> + * @daemon is used to provide a base cred, with the security data derived from
> + * that; if this is "&init_task", they'll be set to 0, no groups, full
> + * capabilities, and no keys.
>   *
>   * The caller may change these controls afterwards if desired.
>   *
> @@ -714,17 +714,16 @@ struct cred *prepare_kernel_cred(struct task_struct *daemon)
>  	const struct cred *old;
>  	struct cred *new;
>  
> +	if (WARN_ON_ONCE(!daemon))
> +		return NULL;
> +
>  	new = kmem_cache_alloc(cred_jar, GFP_KERNEL);
>  	if (!new)
>  		return NULL;
>  
>  	kdebug("prepare_kernel_cred() alloc %p", new);
>  
> -	if (daemon)
> -		old = get_task_cred(daemon);
> -	else
> -		old = get_cred(&init_cred);
> -
> +	old = get_task_cred(daemon);
>  	validate_creds(old);
>  
>  	*new = *old;
> diff --git a/net/dns_resolver/dns_key.c b/net/dns_resolver/dns_key.c
> index 3aced951d5ab..01e54b46ae0b 100644
> --- a/net/dns_resolver/dns_key.c
> +++ b/net/dns_resolver/dns_key.c
> @@ -337,7 +337,7 @@ static int __init init_dns_resolver(void)
>  	 * this is used to prevent malicious redirections from being installed
>  	 * with add_key().
>  	 */
> -	cred = prepare_kernel_cred(NULL);
> +	cred = prepare_kernel_cred(&init_task);
>  	if (!cred)
>  		return -ENOMEM;
>  
Acked-by: Russ Weight <russell.h.weight@intel.com>

- Russ

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2022-10-27 17:16 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-10-26 23:31 [PATCH] cred: Do not default to init_cred in prepare_kernel_cred() Kees Cook
2022-10-27  6:49 ` Greg Kroah-Hartman
2022-10-27 12:03 ` Paulo Alcantara
2022-10-27 13:07 ` Sergey Senozhatsky
2022-10-27 17:03 ` Luis Chamberlain
2022-10-27 17:16 ` Russ Weight

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.