Re: [PATCH 2/7] crypto: Support more ASN.1 types

["Daniel P. Berrangé" <berrange@redhat.com>]

On Mon, Jun 13, 2022 at 04:45:26PM +0800, Lei He wrote:
> 1. support decoding of 'bit string','octet string',
> 'object id', and 'context specific tag' for DER decoder.
> 2. support encoding of int and sequence for DER decoder.
> 3. add test suites for the above changes.
> 
> Signed-off-by: lei he <helei.sig11@bytedance.com>
> ---
>  crypto/der.c                 | 174 +++++++++++++++++++++++++++++++++++++------
>  crypto/der.h                 | 128 ++++++++++++++++++++++++++++++-
>  tests/unit/test-crypto-der.c | 126 +++++++++++++++++++++++++------
>  3 files changed, 382 insertions(+), 46 deletions(-)
> 
> diff --git a/crypto/der.c b/crypto/der.c
> index f877390bbb..edf2c6c313 100644
> --- a/crypto/der.c
> +++ b/crypto/der.c
> @@ -27,15 +27,68 @@ enum QCryptoDERTypeTag {
>      QCRYPTO_DER_TYPE_TAG_INT = 0x2,
>      QCRYPTO_DER_TYPE_TAG_BIT_STR = 0x3,
>      QCRYPTO_DER_TYPE_TAG_OCT_STR = 0x4,
> -    QCRYPTO_DER_TYPE_TAG_OCT_NULL = 0x5,
> -    QCRYPTO_DER_TYPE_TAG_OCT_OID = 0x6,
> +    QCRYPTO_DER_TYPE_TAG_NULL = 0x5,
> +    QCRYPTO_DER_TYPE_TAG_OID = 0x6,
>      QCRYPTO_DER_TYPE_TAG_SEQ = 0x10,
>      QCRYPTO_DER_TYPE_TAG_SET = 0x11,
>  };
>  
> -#define QCRYPTO_DER_CONSTRUCTED_MASK 0x20
> +enum QCryptoDERTagClass {
> +    QCRYPTO_DER_TAG_CLASS_UNIV = 0x0,
> +    QCRYPTO_DER_TAG_CLASS_APPL = 0x1,
> +    QCRYPTO_DER_TAG_CLASS_CONT = 0x2,
> +    QCRYPTO_DER_TAG_CLASS_PRIV = 0x3,
> +};
> +
> +enum QCryptoDERTagEnc {
> +    QCRYPTO_DER_TAG_ENC_PRIM = 0x0,
> +    QCRYPTO_DER_TAG_ENC_CONS = 0x1,
> +};
> +
> +#define QCRYPTO_DER_TAG_ENC_MASK 0x20
> +#define QCRYPTO_DER_TAG_ENC_SHIFT 5
> +
> +#define QCRYPTO_DER_TAG_CLASS_MASK 0xc0
> +#define QCRYPTO_DER_TAG_CLASS_SHIFT 6
> +
> +#define QCRYPTO_DER_TAG_VAL_MASK 0x1f
>  #define QCRYPTO_DER_SHORT_LEN_MASK 0x80
>  
> +#define QCRYPTO_DER_TAG(class, enc, val)        \
> +    (((class) << QCRYPTO_DER_TAG_CLASS_SHIFT) | \
> +     ((enc) << QCRYPTO_DER_TAG_ENC_SHIFT) | val)
> +
> +static void qcrypto_der_encode_data(const uint8_t *src, size_t src_len,
> +                                    uint8_t *dst, size_t *dst_len)
> +{
> +    size_t max_length = 0xFF;
> +    uint8_t length_bytes = 0, header_byte;
> +
> +    if (src_len < QCRYPTO_DER_SHORT_LEN_MASK) {
> +        header_byte = src_len;
> +        *dst_len = src_len + 1;
> +    } else {
> +        for (length_bytes = 1; max_length < src_len;) {
> +            length_bytes++;
> +            max_length = (max_length << 8) + max_length;
> +        }

Can't length_bytes++ be in the for clause like:

         for (length_bytes = 1; max_length < src_len;length_bytes++) {
             max_length = (max_length << 8) + max_length;
         }

?


Aside from that minor nitpick

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|