linux-kernel - Re: [PATCH v2 2/6] s390/uv: Retrieve UV secrets support

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] [day] [month] [year] [list]

Message-ID: <3463a84c-1f68-4d60-b705-3079491f0176@linux.ibm.com>
Date: Mon, 14 Oct 2024 13:46:32 +0200
From: Steffen Eiden <seiden@...ux.ibm.com>
To: Janosch Frank <frankja@...ux.ibm.com>, linux-kernel@...r.kernel.org,
        linux-s390@...r.kernel.org
Cc: Ingo Franzki <ifranzki@...ux.ibm.com>,
        Harald Freudenberger <freude@...ux.ibm.com>,
        Christoph Schlameuss <schlameuss@...ux.ibm.com>,
        Claudio Imbrenda <imbrenda@...ux.ibm.com>
Subject: Re: [PATCH v2 2/6] s390/uv: Retrieve UV secrets support

Thanks for your comments Janosch.

On 10/7/24 2:31 PM, Janosch Frank wrote:
> On 10/2/24 6:05 PM, Steffen Eiden wrote:
>> Provide a kernel API to retrieve secrets from the UV secret store.
>> Add two new functions:
>> * `uv_get_secret_metadata` - get metadata for a given secret identifier
>> * `uv_retrieve_secret` - get the secret value for the secret index
>>
>> With those two functions one can extract the secret for a given secret
>> id, if the secret is retrievable.
>>
>> Signed-off-by: Steffen Eiden <seiden@...ux.ibm.com>
>> ---
>>   arch/s390/include/asm/uv.h | 131 ++++++++++++++++++++++++++++++++++++-
>>   arch/s390/kernel/uv.c      | 127 ++++++++++++++++++++++++++++++++++-
>>   2 files changed, 256 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/s390/include/asm/uv.h b/arch/s390/include/asm/uv.h
>> index 94ff58336e8e..aef333aaaef4 100644
>> --- a/arch/s390/include/asm/uv.h
>> +++ b/arch/s390/include/asm/uv.h
>> @@ -62,6 +62,7 @@
>>   #define UVC_CMD_ADD_SECRET        0x1031
>>   #define UVC_CMD_LIST_SECRETS        0x1033
>>   #define UVC_CMD_LOCK_SECRETS        0x1034
>> +#define UVC_CMD_RETR_SECRET        0x1035
>>   /* Bits in installed uv calls */
>>   enum uv_cmds_inst {
>> @@ -95,6 +96,7 @@ enum uv_cmds_inst {
>>       BIT_UVC_CMD_ADD_SECRET = 29,
>>       BIT_UVC_CMD_LIST_SECRETS = 30,
>>       BIT_UVC_CMD_LOCK_SECRETS = 31,
>> +    BIT_UVC_CMD_RETR_SECRETS = 33,
> 
> One is SECRET and the other is SECRET_S_.
> I know that you coded this according to spec, but sometimes we need to fix the spec. I've contacted the spec authors to fix it on their end if possible.
Yes we should fix the specs.
I will use singular forms on both constants.

> 
> [...]
> 

>> + * Do the actual search for `uv_get_secret_metadata`
>> + *
>> + * Context: might sleep
>> + */
>> +static int find_secret(const u8 secret_id[UV_SECRET_ID_LEN],
>> +               struct uv_secret_list *list,
>> +               struct uv_secret_list_item_hdr *secret)
>> +{
>> +    u16 start_idx = 0;
>> +    u16 list_rc;
>> +    int ret;
>> +
>> +    do {
>> +        uv_list_secrets((u8 *)list, start_idx, &list_rc, NULL);
>> +        if (!(list_rc == UVC_RC_EXECUTED || list_rc == UVC_RC_MORE_DATA)) {
> 
> Inverting this conditional would get rid of 3 characters.
> Did you chose to implement it like this on purpose?
> 
No special purpose behind that. In fact, I prefer your shorter variant.
Thanks, I'll change that.

>> +            if (list_rc == UVC_RC_INV_CMD)
>> +                return -ENODEV;
>> +            else
>> +                return -EIO;
>> +        }
>> +        ret = find_secret_in_page(secret_id, list, secret);
>> +        if (ret == 0)
>> +            return ret;
>> +        start_idx = list->next_secret_idx;
>> +    } while (list_rc == UVC_RC_MORE_DATA && start_idx < list->next_secret_idx);
>> +
>> +    return -ENOENT;
> 
> 

Steffen