[PATCH v2] keys: Protect the update method check against type removal

[PATCH v2] keys: Protect the update method check against type removal

[Cen Zhang]

key_update() reads key->type->update, ->def_datalen and ->preparse
before it takes key->sem. A reference on the key does not pin the
module that provides its type, so KEYCTL_UPDATE can race with the
unloading of that module, for example dns_resolver:

  KEYCTL_UPDATE task                  Module unload and key GC
  ------------------                  ------------------------
  keyctl_update_key()
    key_update()
      key_permission()
      load key->type
                                      exit_dns_resolver()
                                        unregister_key_type()
                                          GC: down_write(&key->sem)
                                          GC: retype to key_type_dead
                                          GC: up_write(&key->sem)
                                        unregister_key_type() returns
                                      free_module()
      read saved type's update member

The final access reads freed module memory. With the updating task
held after the key->type load, it faults:

  BUG: unable to handle page fault for address: fffffbfff804107f
  #PF: supervisor read access in kernel mode
  RIP: 0010:key_update+0x12d/0x550
  Call Trace:
   keyctl_update_key+0x6b/0x110
   __do_sys_keyctl+0x21d/0x4f0

This is the same problem that commit 925c84e3cc21 ("keys: Protect the
read method check against type removal") fixed for KEYCTL_READ.

For dns_resolver, which has no update method, key_update() returns at
the check, so doing the check with key->sem held would be enough. A
key type that is built as a module and has an update method, like
encrypted or trusted, gets past the check. If its module is unloaded
after the check and before key_update() takes key->sem, the GC has
already moved the key to key_type_dead, and key_update() calls the
update method of key_type_dead, which is NULL. So key->sem has to stay
held from the check until the update is done.

Take key->sem before the first access to key->type and hold it until
the preparsed payload has been freed. Once a key exists, only the GC
changes key->type, and it does so with key->sem held for writing.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Cen Zhang <zzzccc427@gmail.com>
---
Changes in v2:
- Refine the commit message.
- Remove the code comment in key_update().
- Drop the Assisted-by tag.

Link to v1: https://lore.kernel.org/r/pm-keys-update-type-removal-20261029-v6-3d92b2f9693854230a02@gmail.com

 security/keys/key.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/security/keys/key.c b/security/keys/key.c
index e88548f1a704..994a34a80c29 100644
--- a/security/keys/key.c
+++ b/security/keys/key.c
@@ -1101,9 +1101,12 @@ int key_update(key_ref_t key_ref, const void *payload, size_t plen)
 	if (ret < 0)
 		return ret;
 
+	down_write(&key->sem);
+
 	/* attempt to update it if supported */
+	ret = -EOPNOTSUPP;
 	if (!key->type->update)
-		return -EOPNOTSUPP;
+		goto out;
 
 	memset(&prep, 0, sizeof(prep));
 	prep.data = payload;
@@ -1116,8 +1119,6 @@ int key_update(key_ref_t key_ref, const void *payload, size_t plen)
 			goto error;
 	}
 
-	down_write(&key->sem);
-
 	ret = key->type->update(key, &prep);
 	if (ret == 0) {
 		/* Updating a negative key positively instantiates it */
@@ -1125,11 +1126,11 @@ int key_update(key_ref_t key_ref, const void *payload, size_t plen)
 		notify_key(key, NOTIFY_KEY_UPDATED, 0);
 	}
 
-	up_write(&key->sem);
-
 error:
 	if (key->type->preparse)
 		key->type->free_preparse(&prep);
+out:
+	up_write(&key->sem);
 	return ret;
 }
 EXPORT_SYMBOL(key_update);

base-commit: 9a2a4c51dccc3168c3dc949fcbaa360a478fa228
-- 
2.43.0

Re: [PATCH v2] keys: Protect the update method check against type removal

[sashiko-bot]

> key_update() reads key->type->update, ->def_datalen and ->preparse
> before it takes key->sem. A reference on the key does not pin the
> module that provides its type, so KEYCTL_UPDATE can race with the
> unloading of that module, for example dns_resolver:
> 
>   KEYCTL_UPDATE task                  Module unload and key GC
>   ------------------                  ------------------------
>   keyctl_update_key()
>     key_update()
>       key_permission()
>       load key->type
>                                       exit_dns_resolver()
>                                         unregister_key_type()
>                                           GC: down_write(&key->sem)
>                                           GC: retype to key_type_dead
> [ ... ]
> the preparsed payload has been freed. Once a key exists, only the GC
> changes key->type, and it does so with key->sem held for writing.
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Cen Zhang <zzzccc427@gmail.com>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/pm-keys-update-type-removal-20261029-v7-e79d113bcff91e140d04@gmail.com?part=1

Re: [PATCH v2] keys: Protect the update method check against type removal

[Jarkko Sakkinen]

On Fri, Oct 30, 2026 at 12:07:08AM +0800, Cen Zhang wrote:
> key_update() reads key->type->update, ->def_datalen and ->preparse
> before it takes key->sem. A reference on the key does not pin the
> module that provides its type, so KEYCTL_UPDATE can race with the
> unloading of that module, for example dns_resolver:
> 
>   KEYCTL_UPDATE task                  Module unload and key GC
>   ------------------                  ------------------------
>   keyctl_update_key()
>     key_update()
>       key_permission()
>       load key->type
>                                       exit_dns_resolver()
>                                         unregister_key_type()
>                                           GC: down_write(&key->sem)
>                                           GC: retype to key_type_dead
>                                           GC: up_write(&key->sem)
>                                         unregister_key_type() returns
>                                       free_module()
>       read saved type's update member
> 
> The final access reads freed module memory. With the updating task
> held after the key->type load, it faults:
> 
>   BUG: unable to handle page fault for address: fffffbfff804107f
>   #PF: supervisor read access in kernel mode
>   RIP: 0010:key_update+0x12d/0x550
>   Call Trace:
>    keyctl_update_key+0x6b/0x110
>    __do_sys_keyctl+0x21d/0x4f0
> 
> This is the same problem that commit 925c84e3cc21 ("keys: Protect the
> read method check against type removal") fixed for KEYCTL_READ.
> 
> For dns_resolver, which has no update method, key_update() returns at
> the check, so doing the check with key->sem held would be enough. A
> key type that is built as a module and has an update method, like
> encrypted or trusted, gets past the check. If its module is unloaded
> after the check and before key_update() takes key->sem, the GC has
> already moved the key to key_type_dead, and key_update() calls the
> update method of key_type_dead, which is NULL. So key->sem has to stay
> held from the check until the update is done.
> 
> Take key->sem before the first access to key->type and hold it until
> the preparsed payload has been freed. Once a key exists, only the GC
> changes key->type, and it does so with key->sem held for writing.
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Cen Zhang <zzzccc427@gmail.com>
> ---
> Changes in v2:
> - Refine the commit message.
> - Remove the code comment in key_update().
> - Drop the Assisted-by tag.
> 
> Link to v1: https://lore.kernel.org/r/pm-keys-update-type-removal-20261029-v6-3d92b2f9693854230a02@gmail.com
> 
>  security/keys/key.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/security/keys/key.c b/security/keys/key.c
> index e88548f1a704..994a34a80c29 100644
> --- a/security/keys/key.c
> +++ b/security/keys/key.c
> @@ -1101,9 +1101,12 @@ int key_update(key_ref_t key_ref, const void *payload, size_t plen)
>  	if (ret < 0)
>  		return ret;
>  
> +	down_write(&key->sem);
> +
>  	/* attempt to update it if supported */
> +	ret = -EOPNOTSUPP;
>  	if (!key->type->update)
> -		return -EOPNOTSUPP;
> +		goto out;
>  
>  	memset(&prep, 0, sizeof(prep));
>  	prep.data = payload;
> @@ -1116,8 +1119,6 @@ int key_update(key_ref_t key_ref, const void *payload, size_t plen)
>  			goto error;
>  	}
>  
> -	down_write(&key->sem);
> -
>  	ret = key->type->update(key, &prep);
>  	if (ret == 0) {
>  		/* Updating a negative key positively instantiates it */
> @@ -1125,11 +1126,11 @@ int key_update(key_ref_t key_ref, const void *payload, size_t plen)
>  		notify_key(key, NOTIFY_KEY_UPDATED, 0);
>  	}
>  
> -	up_write(&key->sem);
> -
>  error:
>  	if (key->type->preparse)
>  		key->type->free_preparse(&prep);

Hmm.. a nitpick but here should be empty line, or at is least common
to have that before the next label.

> +out:
> +	up_write(&key->sem);

And also these labels don't have coherency in naming. Not claiming
there is overall but it would be good to start improving the
situation.

So I think this would add more clarity:

out_free_preparse:
        if (key->type->preparse)
                key->type->free_preparse(&prep);

out:
        up_write(&key->sem);
        return ret;
}
EXPORT_SYMBOL(key_update);

And is more aligned with our defacto conventions.

>  	return ret;
>  }
>  EXPORT_SYMBOL(key_update);
> 
> base-commit: 9a2a4c51dccc3168c3dc949fcbaa360a478fa228
> -- 
> 2.43.0
> 
Br, Jarkko