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

[PATCH] 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.

The encrypted and trusted types can be built as modules and have an
update method. For them there is a second window: the GC can move the
key to key_type_dead after the method check and before key_update()
takes key->sem. key_update() then calls the NULL update method of
key_type_dead.

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.
unregister_key_type() does not return before the GC has done that for
every key of the type that is still in use, so the type stays valid
for the whole update. If the GC has already moved the key to
key_type_dead when key_update() gets key->sem, there is no update
method and key_update() returns -EOPNOTSUPP.

Assisted-by: LLM
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Cen Zhang <zzzccc427@gmail.com>
---
 security/keys/key.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/security/keys/key.c b/security/keys/key.c
index e88548f1a704..bac2333089fc 100644
--- a/security/keys/key.c
+++ b/security/keys/key.c
@@ -1101,9 +1101,13 @@ int key_update(key_ref_t key_ref, const void *payload, size_t plen)
 	if (ret < 0)
 		return ret;
 
+	/* the GC cannot change the key's type while we hold the semaphore */
+	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 +1120,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 +1127,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] 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
> [ ... ]
> method and key_update() returns -EOPNOTSUPP.
> 
> Assisted-by: LLM
> 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-v6-3d92b2f9693854230a02@gmail.com?part=1

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

[Jarkko Sakkinen]

On Thu, Oct 29, 2026 at 07:58:06PM +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.

This is useful bit, thanks.

> 
> The encrypted and trusted types can be built as modules and have an
> update method. For them there is a second window: the GC can move the

How is the second window related to the fact that encrypted and trusted
can be built as LKMs and where is "there"?

> key to key_type_dead after the method check and before key_update()
> takes key->sem. key_update() then calls the NULL update method of
> key_type_dead.

I don't understand this paragrap overall. Needs a rewrite.

> 
> 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.

+1

> unregister_key_type() does not return before the GC has done that for
> every key of the type that is still in use, so the type stays valid
> for the whole update. If the GC has already moved the key to
> key_type_dead when key_update() gets key->sem, there is no update
> method and key_update() returns -EOPNOTSUPP.

I don't understand this but the difference to the previous case is that
it would be better to be just deleted.

> 
> Assisted-by: LLM

Please run checkpatch.pl --strict for the next version.

> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Cen Zhang <zzzccc427@gmail.com>
> ---
>  security/keys/key.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/security/keys/key.c b/security/keys/key.c
> index e88548f1a704..bac2333089fc 100644
> --- a/security/keys/key.c
> +++ b/security/keys/key.c
> @@ -1101,9 +1101,13 @@ int key_update(key_ref_t key_ref, const void *payload, size_t plen)
>  	if (ret < 0)
>  		return ret;
>  
> +	/* the GC cannot change the key's type while we hold the semaphore */

I don't understand this.

> +	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 +1120,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 +1127,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
> 

Br, Jarkko

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

[Cen Zhang]

Hi Jarkko,

Jarkko Sakkinen <jarkko@kernel.org> 于2026年10月29日周四 21:14写道：
>
> On Thu, Oct 29, 2026 at 07:58:06PM +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.
>
> This is useful bit, thanks.

Thanks for the review.

>
> >
> > The encrypted and trusted types can be built as modules and have an
> > update method. For them there is a second window: the GC can move the
>
> How is the second window related to the fact that encrypted and trusted
> can be built as LKMs and where is "there"?
>
> > key to key_type_dead after the method check and before key_update()
> > takes key->sem. key_update() then calls the NULL update method of
> > key_type_dead.
>
> I don't understand this paragrap overall. Needs a rewrite.

Sorry for that. Perhaps this wording would be better:

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.

> >
> > Assisted-by: LLM
>
> Please run checkpatch.pl --strict for the next version.

Thanks, I see that the Assisted-by tag has been removed from the
documentation and checkpatch. I will drop it in v2.

>
> > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > Signed-off-by: Cen Zhang <zzzccc427@gmail.com>
> > ---
> >  security/keys/key.c | 12 +++++++-----
> >  1 file changed, 7 insertions(+), 5 deletions(-)
> >
> > diff --git a/security/keys/key.c b/security/keys/key.c
> > index e88548f1a704..bac2333089fc 100644
> > --- a/security/keys/key.c
> > +++ b/security/keys/key.c
> > @@ -1101,9 +1101,13 @@ int key_update(key_ref_t key_ref, const void *payload, size_t plen)
> >       if (ret < 0)
> >               return ret;
> >
> > +     /* the GC cannot change the key's type while we hold the semaphore */
>
> I don't understand this.

I will remove it in v2.

Best regards,
Cen Zhang

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

[Jarkko Sakkinen]

On Thu, Oct 29, 2026 at 10:04:13PM +0800, Cen Zhang wrote:
> Hi Jarkko,
> 
> Jarkko Sakkinen <jarkko@kernel.org> 于2026年10月29日周四 21:14写道：
> >
> > On Thu, Oct 29, 2026 at 07:58:06PM +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.
> >
> > This is useful bit, thanks.
> 
> Thanks for the review.
> 
> >
> > >
> > > The encrypted and trusted types can be built as modules and have an
> > > update method. For them there is a second window: the GC can move the
> >
> > How is the second window related to the fact that encrypted and trusted
> > can be built as LKMs and where is "there"?
> >
> > > key to key_type_dead after the method check and before key_update()
> > > takes key->sem. key_update() then calls the NULL update method of
> > > key_type_dead.
> >
> > I don't understand this paragrap overall. Needs a rewrite.
> 
> Sorry for that. Perhaps this wording would be better:
> 
> 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.
> 
> > >
> > > Assisted-by: LLM
> >
> > Please run checkpatch.pl --strict for the next version.
> 
> Thanks, I see that the Assisted-by tag has been removed from the
> documentation and checkpatch. I will drop it in v2.

Yeah, overall I think this is improving and that's why I'm nitpicking
stuff. What I would wish that if you could hold with any other fixes up
until we've got the previous fully addressed.

I mean submitting a fix trial that is not correct is not a problem
or wrong for that matter. The problem is that if you put stuff out in
volumes with patches in various phases, the odds that fix will land
to the mainline tree rise.

This is exactly what I got pissed of last time. Just wanted to this
clear this one up. Also, you can use that on-hold time to re-check
your unsubmitted patches one more time i.e., do your job better
;-) At leas for me that is something that I want to continuously
improve.

Br, Jarkko

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

[Cen Zhang]

Hi Jarkko,

Jarkko Sakkinen <jarkko@kernel.org> 于2026年10月29日周四 23:04写道：
>
> Yeah, overall I think this is improving and that's why I'm nitpicking
> stuff. What I would wish that if you could hold with any other fixes up
> until we've got the previous fully addressed.
>
> I mean submitting a fix trial that is not correct is not a problem
> or wrong for that matter. The problem is that if you put stuff out in
> volumes with patches in various phases, the odds that fix will land
> to the mainline tree rise.
>
> This is exactly what I got pissed of last time. Just wanted to this
> clear this one up. Also, you can use that on-hold time to re-check
> your unsubmitted patches one more time i.e., do your job better
> ;-) At leas for me that is something that I want to continuously
> improve.
>
> Br, Jarkko
>

Thanks for the guidence. I will follow that.

Best regards,
Cen Zhang