Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Organization: Red Hat UK Ltd. Registered Address: Red Hat UK Ltd, Amberley
        Place, 107-111 Peascod Street, Windsor, Berkshire, SI4 1TE, United
        Kingdom.
        Registered in England and Wales under Company Registration No. 3798903
From:   David Howells <dhowells@redhat.com>
In-Reply-To: <20230323130412.32097-1-petr.pavlu@suse.com>
References: <20230323130412.32097-1-petr.pavlu@suse.com>
To:     Petr Pavlu <petr.pavlu@suse.com>
Cc:     dhowells@redhat.com, jarkko@kernel.org, keyrings@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] keys: Fix linking a duplicate key to a keyring's assoc_array
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <2413880.1686233574.1@warthog.procyon.org.uk>
Date:   Thu, 08 Jun 2023 15:12:54 +0100
Message-ID: <2413881.1686233574@warthog.procyon.org.uk>
Precedence: bulk

Apologies for missing this patch.

Petr Pavlu <petr.pavlu@suse.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