Received-SPF: pass (google.com: domain of linux-crypto-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH 05/11] nvme: add definitions for NVMe In-Band
 authentication
To:     Sagi Grimberg <sagi@grimberg.me>, Christoph Hellwig <hch@lst.de>
Cc:     Keith Busch <keith.busch@wdc.com>, linux-nvme@lists.infradead.org,
        Herbert Xu <herbert@gondor.apana.org.au>,
        "David S . Miller" <davem@davemloft.net>,
        linux-crypto@vger.kernel.org
References: <20210716110428.9727-1-hare@suse.de>
 <20210716110428.9727-6-hare@suse.de>
 <affd7d4a-6eb5-20b4-fcb3-7e8a0767d7a2@grimberg.me>
From:   Hannes Reinecke <hare@suse.de>
Message-ID: <2eebe51b-7ca0-c2cf-0949-18fb88a0395b@suse.de>
Date:   Sat, 17 Jul 2021 16:04:31 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.11.0
MIME-Version: 1.0
In-Reply-To: <affd7d4a-6eb5-20b4-fcb3-7e8a0767d7a2@grimberg.me>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 7/17/21 8:30 AM, Sagi Grimberg wrote:
> 
> 
> On 7/16/21 4:04 AM, Hannes Reinecke wrote:
>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>> ---
>>   include/linux/nvme.h | 185 ++++++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 184 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/linux/nvme.h b/include/linux/nvme.h
>> index b7c4c4130b65..7b94abacfd08 100644
>> --- a/include/linux/nvme.h
>> +++ b/include/linux/nvme.h
>> @@ -19,6 +19,7 @@
>>   #define NVMF_TRSVCID_SIZE    32
>>   #define NVMF_TRADDR_SIZE    256
>>   #define NVMF_TSAS_SIZE        256
>> +#define NVMF_AUTH_HASH_LEN    64
>>   #define NVME_DISC_SUBSYS_NAME    "nqn.2014-08.org.nvmexpress.discovery"
>> @@ -1263,6 +1264,8 @@ enum nvmf_capsule_command {
>>       nvme_fabrics_type_property_set    = 0x00,
>>       nvme_fabrics_type_connect    = 0x01,
>>       nvme_fabrics_type_property_get    = 0x04,
>> +    nvme_fabrics_type_auth_send    = 0x05,
>> +    nvme_fabrics_type_auth_receive    = 0x06,
>>   };
>>   #define nvme_fabrics_type_name(type)   { type, #type }
>> @@ -1270,7 +1273,9 @@ enum nvmf_capsule_command {
>>       __print_symbolic(type,                        \
>>           nvme_fabrics_type_name(nvme_fabrics_type_property_set),    \
>>           nvme_fabrics_type_name(nvme_fabrics_type_connect),    \
>> -        nvme_fabrics_type_name(nvme_fabrics_type_property_get))
>> +        nvme_fabrics_type_name(nvme_fabrics_type_property_get), \
>> +        nvme_fabrics_type_name(nvme_fabrics_type_auth_send),    \
>> +        nvme_fabrics_type_name(nvme_fabrics_type_auth_receive))
>>   /*
>>    * If not fabrics command, fctype will be ignored.
>> @@ -1393,6 +1398,182 @@ struct nvmf_property_get_command {
>>       __u8        resv4[16];
>>   };
>> +struct nvmf_auth_send_command {
>> +    __u8        opcode;
>> +    __u8        resv1;
>> +    __u16        command_id;
>> +    __u8        fctype;
>> +    __u8        resv2[19];
>> +    union nvme_data_ptr dptr;
>> +    __u8        resv3;
>> +    __u8        spsp0;
>> +    __u8        spsp1;
>> +    __u8        secp;
>> +    __le32        tl;
>> +    __u8        resv4[12];
> 
> Isn't that 16 bytes?
> You should add these to the compile time checkers
> in _nvme_check_size.
> 

If you say so ... I'll cross-check.

>> +
>> +};
>> +
>> +struct nvmf_auth_receive_command {
>> +    __u8        opcode;
>> +    __u8        resv1;
>> +    __u16        command_id;
>> +    __u8        fctype;
>> +    __u8        resv2[19];
>> +    union nvme_data_ptr dptr;
>> +    __u8        resv3;
>> +    __u8        spsp0;
>> +    __u8        spsp1;
>> +    __u8        secp;
>> +    __le32        al;
>> +    __u8        resv4[12];
>> +};
>> +
>> +/* Value for secp */
>> +enum {
>> +    NVME_AUTH_DHCHAP_PROTOCOL_IDENTIFIER    = 0xe9,
>> +};
>> +
>> +/* Defined value for auth_type */
>> +enum {
>> +    NVME_AUTH_COMMON_MESSAGES    = 0x00,
>> +    NVME_AUTH_DHCHAP_MESSAGES    = 0x01,
>> +};
>> +
>> +/* Defined messages for auth_id */
>> +enum {
>> +    NVME_AUTH_DHCHAP_MESSAGE_NEGOTIATE    = 0x00,
>> +    NVME_AUTH_DHCHAP_MESSAGE_CHALLENGE    = 0x01,
>> +    NVME_AUTH_DHCHAP_MESSAGE_REPLY        = 0x02,
>> +    NVME_AUTH_DHCHAP_MESSAGE_SUCCESS1    = 0x03,
>> +    NVME_AUTH_DHCHAP_MESSAGE_SUCCESS2    = 0x04,
>> +    NVME_AUTH_DHCHAP_MESSAGE_FAILURE2    = 0xf0,
>> +    NVME_AUTH_DHCHAP_MESSAGE_FAILURE1    = 0xf1,
>> +};
>> +
>> +struct nvmf_auth_dhchap_protocol_descriptor {
>> +    __u8        authid;
>> +    __u8        rsvd;
>> +    __u8        halen;
>> +    __u8        dhlen;
>> +    __u8        idlist[60];
>> +};
>> +
>> +enum {
>> +    NVME_AUTH_DHCHAP_AUTH_ID    = 0x01,
>> +};
>> +
>> +enum {
>> +    NVME_AUTH_DHCHAP_HASH_SHA256    = 0x01,
> 
> Maybe s/HASH/HF/ (stands for hash function, which is
> a better description).
> 

Or HMAC, as this is what it's used for...

>> +    NVME_AUTH_DHCHAP_HASH_SHA384    = 0x02,
>> +    NVME_AUTH_DHCHAP_HASH_SHA512    = 0x03,
>> +};
>> +
>> +enum {
>> +    NVME_AUTH_DHCHAP_DHGROUP_NULL    = 0x00,
>> +    NVME_AUTH_DHCHAP_DHGROUP_2048    = 0x01,
>> +    NVME_AUTH_DHCHAP_DHGROUP_3072    = 0x02,
>> +    NVME_AUTH_DHCHAP_DHGROUP_4096    = 0x03,
>> +    NVME_AUTH_DHCHAP_DHGROUP_6144    = 0x04,
>> +    NVME_AUTH_DHCHAP_DHGROUP_8192    = 0x05,
>> +};
>> +
>> +union nvmf_auth_protocol {
>> +    struct nvmf_auth_dhchap_protocol_descriptor dhchap;
>> +};
>> +
>> +struct nvmf_auth_dhchap_negotiate_data {
>> +    __u8        auth_type;
>> +    __u8        auth_id;
>> +    __u8        rsvd[2];
>> +    __le16        t_id;
>> +    __u8        sc_c;
>> +    __u8        napd;
>> +    union nvmf_auth_protocol auth_protocol[];
>> +};
>> +
>> +struct nvmf_auth_dhchap_challenge_data {
>> +    __u8        auth_type;
>> +    __u8        auth_id;
>> +    __u8        rsvd1[2];
>> +    __le16        t_id;
>> +    __u8        hl;
>> +    __u8        rsvd2;
>> +    __u8        hashid;
>> +    __u8        dhgid;
>> +    __le16        dhvlen;
>> +    __le32        seqnum;
>> +    /* 'hl' bytes of challenge value */
>> +    __u8        cval[];
>> +    /* followed by 'dhvlen' bytes of DH value */
>> +};
>> +
>> +struct nvmf_auth_dhchap_reply_data {
>> +    __u8        auth_type;
>> +    __u8        auth_id;
>> +    __u8        rsvd1[2];
> 
> Maybe __u32 rsvd1? Usually its done this way in the other
> headers...
> 

Ah. Right, will fix.

>> +    __le16        t_id;
>> +    __u8        hl;
>> +    __u8        rsvd2;
>> +    __u8        cvalid;
>> +    __u8        rsvd3;
>> +    __le16        dhvlen;
>> +    __le32        seqnum;
>> +    /* 'hl' bytes of response data */
>> +    __u8        rval[];
>> +    /* followed by 'hl' bytes of Challenge value */
>> +    /* followed by 'dhvlen' bytes of DH value */
>> +};
>> +
>> +enum {
>> +    NVME_AUTH_DHCHAP_RESPONSE_VALID    = (1 << 0),
>> +};
>> +
>> +struct nvmf_auth_dhchap_success1_data {
>> +    __u8        auth_type;
>> +    __u8        auth_id;
>> +    __u8        rsvd1[2];
>> +    __le16        t_id;
>> +    __u8        hl;
>> +    __u8        rsvd2;
>> +    __u8        rvalid;
>> +    __u8        rsvd3[7];
>> +    /* 'hl' bytes of response value if 'rvalid' is set */
>> +    __u8        rval[];
> 
> It really sucks that we have zero-length pointers in
> a wire-format struct... but anyways, it is what it is...
> 

Yeah, and that's not the worst of it; the 'reply' structure has _three_ 
zero-length pointers.
Makes me wonder if I should drop them completely...

>> +};
>> +
>> +struct nvmf_auth_dhchap_success2_data {
>> +    __u8        auth_type;
>> +    __u8        auth_id;
>> +    __u8        rsvd1[2];
>> +    __le16        t_id;
>> +    __u8        rsvd2[10];
>> +};
>> +
>> +struct nvmf_auth_dhchap_failure_data {
>> +    __u8        auth_type;
>> +    __u8        auth_id;
>> +    __u8        rsvd1[2];
>> +    __le16        t_id;
>> +    __u8        reason_code;
>> +    __u8        reason_code_explanation;
> 
> I'd maybe do those shorter;
> rescode
> rescode_exp
> 

Ok.

>> +};
>> +
>> +enum {
>> +    NVME_AUTH_DHCHAP_FAILURE_REASON_FAILED    = 0x01,
>> +};
>> +
>> +enum {
>> +    NVME_AUTH_DHCHAP_FAILURE_FAILED            = 0x01,
>> +    NVME_AUTH_DHCHAP_FAILURE_NOT_USABLE        = 0x02,
>> +    NVME_AUTH_DHCHAP_FAILURE_CONCAT_MISMATCH    = 0x03,
>> +    NVME_AUTH_DHCHAP_FAILURE_HASH_UNUSABLE        = 0x04,
>> +    NVME_AUTH_DHCHAP_FAILURE_DHGROUP_UNUSABLE    = 0x05,
>> +    NVME_AUTH_DHCHAP_FAILURE_INVALID_PAYLOAD    = 0x06,
>> +    NVME_AUTH_DHCHAP_FAILURE_INVALID_MESSAGE    = 0x07,
> 
> I think the language in the spec is "incorrect", why not
> stick with that instead of "invalid"?
> 

Ah. Things got changed so much during development of the spec that I 
seem to have lost track. Will change it.

>> +};
>> +
>> +
>>   struct nvme_dbbuf {
>>       __u8            opcode;
>>       __u8            flags;
>> @@ -1436,6 +1617,8 @@ struct nvme_command {
>>           struct nvmf_connect_command connect;
>>           struct nvmf_property_set_command prop_set;
>>           struct nvmf_property_get_command prop_get;
>> +        struct nvmf_auth_send_command auth_send;
>> +        struct nvmf_auth_receive_command auth_receive;
>>           struct nvme_dbbuf dbbuf;
>>           struct nvme_directive_cmd directive;
>>       };
>>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer