linux-kernel - Re: [PATCH] keys: Fix linking a duplicate key to a keyring's assoc

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 08 Jun 2023 15:12:54 +0100
From:   David Howells <dhowells@...hat.com>
To:     Petr Pavlu <petr.pavlu@...e.com>
Cc:     dhowells@...hat.com, jarkko@...nel.org, keyrings@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] keys: Fix linking a duplicate key to a keyring's assoc_array

Apologies for missing this patch.

Petr Pavlu <petr.pavlu@...e.com> wrote:

> * Back on the first task, function construct_alloc_key() first runs
>   __key_link_begin() to determine an assoc_array_edit operation to
>   insert a new key. Index keys in the array are compared exactly as-is,
>   using keyring_compare_object(). The operation finds that "abcdef" is
>   not yet present in the destination keyring.

Good catch, but I think it's probably the wrong solution.

keyring_compare_object() needs to use the ->cmp() function from the key type.

It's not just request_key() that might have a problem, but also key_link().

There are also asymmetric keys which match against multiple criteria - though
I'm fine with just matching the main description there (the important bit
being to maintain the integrity of the tree inside assoc_array).

I wonder, should I scrap the assoc_array approach and go to each keyring being
a linked-list?  It's slower, but a lot easier to manage - and more forgiving
of problems.

	struct key_ptr {
		struct list_head	link;
		struct key		*key;
		unsigned long		key_hash;
	};

I'm also wondering if I should remove the key type from the matching criteria
- i.e. there can only be one key with any particular description in a ring at
once, regardless of type.  Unfortunately, this is may be a UAPI breaker
somewhere.

Any thoughts?

David