Message-ID: <20210322064712.GD1667@kadam>
Date:   Mon, 22 Mar 2021 09:47:13 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     Namjae Jeon <namjae.jeon@...sung.com>
Cc:     linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-cifs@...r.kernel.org,
        linux-cifsd-devel@...ts.sourceforge.net, smfrench@...il.com,
        senozhatsky@...omium.org, hyc.lee@...il.com,
        viro@...iv.linux.org.uk, hch@....de, hch@...radead.org,
        ronniesahlberg@...il.com, aurelien.aptel@...il.com,
        aaptel@...e.com, sandeen@...deen.net, colin.king@...onical.com,
        rdunlap@...radead.org,
        Sergey Senozhatsky <sergey.senozhatsky@...il.com>,
        Steve French <stfrench@...rosoft.com>
Subject: Re: [PATCH 2/5] cifsd: add server-side procedures for SMB3

On Mon, Mar 22, 2021 at 02:13:41PM +0900, Namjae Jeon wrote:
> +static unsigned char
> +asn1_octet_decode(struct asn1_ctx *ctx, unsigned char *ch)
> +{
> +	if (ctx->pointer >= ctx->end) {
> +		ctx->error = ASN1_ERR_DEC_EMPTY;
> +		return 0;
> +	}
> +	*ch = *(ctx->pointer)++;
> +	return 1;
> +}


Make this bool.

> +
> +static unsigned char
> +asn1_tag_decode(struct asn1_ctx *ctx, unsigned int *tag)
> +{
> +	unsigned char ch;
> +
> +	*tag = 0;
> +
> +	do {
> +		if (!asn1_octet_decode(ctx, &ch))
> +			return 0;
> +		*tag <<= 7;
> +		*tag |= ch & 0x7F;
> +	} while ((ch & 0x80) == 0x80);
> +	return 1;
> +}

Bool.

> +
> +static unsigned char
> +asn1_id_decode(struct asn1_ctx *ctx,
> +	       unsigned int *cls, unsigned int *con, unsigned int *tag)
> +{
> +	unsigned char ch;
> +
> +	if (!asn1_octet_decode(ctx, &ch))
> +		return 0;
> +
> +	*cls = (ch & 0xC0) >> 6;
> +	*con = (ch & 0x20) >> 5;
> +	*tag = (ch & 0x1F);
> +
> +	if (*tag == 0x1F) {
> +		if (!asn1_tag_decode(ctx, tag))
> +			return 0;
> +	}
> +	return 1;
> +}


Same.

> +
> +static unsigned char
> +asn1_length_decode(struct asn1_ctx *ctx, unsigned int *def, unsigned int *len)
> +{
> +	unsigned char ch, cnt;
> +
> +	if (!asn1_octet_decode(ctx, &ch))
> +		return 0;
> +
> +	if (ch == 0x80)
> +		*def = 0;
> +	else {
> +		*def = 1;
> +
> +		if (ch < 0x80)
> +			*len = ch;
> +		else {
> +			cnt = (unsigned char) (ch & 0x7F);
> +			*len = 0;
> +
> +			while (cnt > 0) {
> +				if (!asn1_octet_decode(ctx, &ch))
> +					return 0;
> +				*len <<= 8;
> +				*len |= ch;
> +				cnt--;
> +			}
> +		}
> +	}
> +
> +	/* don't trust len bigger than ctx buffer */
> +	if (*len > ctx->end - ctx->pointer)
> +		return 0;
> +
> +	return 1;
> +}


Same etc for all.

> +
> +static unsigned char
> +asn1_header_decode(struct asn1_ctx *ctx,
> +		   unsigned char **eoc,
> +		   unsigned int *cls, unsigned int *con, unsigned int *tag)
> +{
> +	unsigned int def = 0;
> +	unsigned int len = 0;
> +
> +	if (!asn1_id_decode(ctx, cls, con, tag))
> +		return 0;
> +
> +	if (!asn1_length_decode(ctx, &def, &len))
> +		return 0;
> +
> +	/* primitive shall be definite, indefinite shall be constructed */
> +	if (*con == ASN1_PRI && !def)
> +		return 0;
> +
> +	if (def)
> +		*eoc = ctx->pointer + len;
> +	else
> +		*eoc = NULL;
> +	return 1;
> +}
> +
> +static unsigned char
> +asn1_eoc_decode(struct asn1_ctx *ctx, unsigned char *eoc)
> +{
> +	unsigned char ch;
> +
> +	if (!eoc) {
> +		if (!asn1_octet_decode(ctx, &ch))
> +			return 0;
> +
> +		if (ch != 0x00) {
> +			ctx->error = ASN1_ERR_DEC_EOC_MISMATCH;
> +			return 0;
> +		}
> +
> +		if (!asn1_octet_decode(ctx, &ch))
> +			return 0;
> +
> +		if (ch != 0x00) {
> +			ctx->error = ASN1_ERR_DEC_EOC_MISMATCH;
> +			return 0;
> +		}
> +	} else {
> +		if (ctx->pointer != eoc) {
> +			ctx->error = ASN1_ERR_DEC_LENGTH_MISMATCH;
> +			return 0;
> +		}
> +	}
> +	return 1;
> +}
> +
> +static unsigned char
> +asn1_subid_decode(struct asn1_ctx *ctx, unsigned long *subid)
> +{
> +	unsigned char ch;
> +
> +	*subid = 0;
> +
> +	do {
> +		if (!asn1_octet_decode(ctx, &ch))
> +			return 0;
> +
> +		*subid <<= 7;
> +		*subid |= ch & 0x7F;
> +	} while ((ch & 0x80) == 0x80);
> +	return 1;
> +}
> +
> +static int
> +asn1_oid_decode(struct asn1_ctx *ctx,
> +		unsigned char *eoc, unsigned long **oid, unsigned int *len)
> +{
> +	unsigned long subid;
> +	unsigned int size;
> +	unsigned long *optr;
> +
> +	size = eoc - ctx->pointer + 1;
> +
> +	/* first subid actually encodes first two subids */
> +	if (size < 2 || size > UINT_MAX/sizeof(unsigned long))
> +		return 0;
> +
> +	*oid = kmalloc(size * sizeof(unsigned long), GFP_KERNEL);
> +	if (!*oid)
> +		return 0;
> +
> +	optr = *oid;
> +
> +	if (!asn1_subid_decode(ctx, &subid)) {
> +		kfree(*oid);
> +		*oid = NULL;
> +		return 0;
> +	}
> +
> +	if (subid < 40) {
> +		optr[0] = 0;
> +		optr[1] = subid;
> +	} else if (subid < 80) {
> +		optr[0] = 1;
> +		optr[1] = subid - 40;
> +	} else {
> +		optr[0] = 2;
> +		optr[1] = subid - 80;
> +	}
> +
> +	*len = 2;
> +	optr += 2;
> +
> +	while (ctx->pointer < eoc) {
> +		if (++(*len) > size) {
> +			ctx->error = ASN1_ERR_DEC_BADVALUE;
> +			kfree(*oid);
> +			*oid = NULL;
> +			return 0;
> +		}
> +
> +		if (!asn1_subid_decode(ctx, optr++)) {
> +			kfree(*oid);
> +			*oid = NULL;
> +			return 0;
> +		}
> +	}
> +	return 1;
> +}

Still bool.

> +
> +static int
> +compare_oid(unsigned long *oid1, unsigned int oid1len,
> +	    unsigned long *oid2, unsigned int oid2len)
> +{
> +	unsigned int i;
> +
> +	if (oid1len != oid2len)
> +		return 0;
> +
> +	for (i = 0; i < oid1len; i++) {
> +		if (oid1[i] != oid2[i])
> +			return 0;
> +	}
> +	return 1;
> +}

Call this oid_eq()?


> +
> +/* BB check for endian conversion issues here */
> +
> +int
> +ksmbd_decode_negTokenInit(unsigned char *security_blob, int length,
> +		    struct ksmbd_conn *conn)
> +{
> +	struct asn1_ctx ctx;
> +	unsigned char *end;
> +	unsigned char *sequence_end;
> +	unsigned long *oid = NULL;
> +	unsigned int cls, con, tag, oidlen, rc, mechTokenlen;
> +	unsigned int mech_type;
> +
> +	ksmbd_debug(AUTH, "Received SecBlob: length %d\n", length);
> +
> +	asn1_open(&ctx, security_blob, length);
> +
> +	/* GSSAPI header */
> +	if (asn1_header_decode(&ctx, &end, &cls, &con, &tag) == 0) {
> +		ksmbd_debug(AUTH, "Error decoding negTokenInit header\n");
> +		return 0;
> +	} else if ((cls != ASN1_APL) || (con != ASN1_CON)

No need for else after a return 0;  Surely, checkpatch complains about
|| on the following line and the extra parentheses?

	if (asn1_header_decode(&ctx, &end, &cls, &con, &tag) == 0) {
		ksmbd_debug(AUTH, "Error decoding negTokenInit header\n");
		return false;
	}

	if (cls != ASN1_APL || con != ASN1_CON || tag != ASN1_EOC) {
		ksmbd_debug(AUTH, "cls = %d con = %d tag = %d\n", cls, con,
			    tag);
		return false;
	}

> +		   || (tag != ASN1_EOC)) {
> +		ksmbd_debug(AUTH, "cls = %d con = %d tag = %d\n", cls, con,
> +			tag);
> +		return 0;
> +	}
> +
> +	/* Check for SPNEGO OID -- remember to free obj->oid */
> +	rc = asn1_header_decode(&ctx, &end, &cls, &con, &tag);
> +	if (rc) {

This code confused the me at first.  I've always assumed "rc" stands for
"return code" but asn1_header_decode() doesn't return error codes, it
returns true false.  Alway do failure handling, instead of success
handling.  That way when you're reading the code you can just read the
code indented one tab to see what it does and the code indented two
tabs to see how the error handling works.

Good:

	frob();
	if (fail)
		clean up();
	frob();
	if (fail)
		clean up();

Bad:
	frob();
	if (success)
		frob();
		if (success)
			frob();
			if (success)
				frob();
		else
			fail = 1;
	if (fail)
		clean up();

So this code confused me.  Keep the ordering consistent with cls, con,
and tag.  In fact just write it exactly like the lines before.

	if (!asn1_header_decode(&ctx, &end, &cls, &con, &tag)) {
		ksmbd_debug(AUTH, "Error decoding negTokenInit header\n");
		return false;
	}

	if (cls != ASN1_UNI || con != ASN1_PRI || tag != ASN1_OJI) {
		ksmbd_debug(AUTH, "cls = %d con = %d tag = %d\n", cls, con,
			    tag);
		return false;
	}

	if (!asn1_oid_decode(&ctx, end, &oid, &oidlen))
		return false;
	if (!oid_equiv()) {
		free();
		return false;
	}

	kfree(oid); <-- I added this

Add a kfree(oid) to the success path to avoid a memory leak.

> +		if ((tag == ASN1_OJI) && (con == ASN1_PRI) &&
> +		    (cls == ASN1_UNI)) {
> +			rc = asn1_oid_decode(&ctx, end, &oid, &oidlen);
> +			if (rc) {
> +				rc = compare_oid(oid, oidlen, SPNEGO_OID,
> +						 SPNEGO_OID_LEN);
> +				kfree(oid);
> +			}
> +		} else
> +			rc = 0;
> +	}
> +
> +	/* SPNEGO OID not present or garbled -- bail out */
> +	if (!rc) {
> +		ksmbd_debug(AUTH, "Error decoding negTokenInit header\n");
> +		return 0;
> +	}
> +
> +	/* SPNEGO */
> +	if (asn1_header_decode(&ctx, &end, &cls, &con, &tag) == 0) {
> +		ksmbd_debug(AUTH, "Error decoding negTokenInit\n");
> +		return 0;
> +	} else if ((cls != ASN1_CTX) || (con != ASN1_CON)
> +		   || (tag != ASN1_EOC)) {
> +		ksmbd_debug(AUTH,
> +			"cls = %d con = %d tag = %d end = %p (%d) exit 0\n",
> +			cls, con, tag, end, *end);
> +		return 0;
> +	}
> +
> +	/* negTokenInit */
> +	if (asn1_header_decode(&ctx, &end, &cls, &con, &tag) == 0) {
> +		ksmbd_debug(AUTH, "Error decoding negTokenInit\n");
> +		return 0;
> +	} else if ((cls != ASN1_UNI) || (con != ASN1_CON)
> +		   || (tag != ASN1_SEQ)) {
> +		ksmbd_debug(AUTH,
> +			"cls = %d con = %d tag = %d end = %p (%d) exit 1\n",
> +			cls, con, tag, end, *end);
> +		return 0;
> +	}
> +
> +	/* sequence */
> +	if (asn1_header_decode(&ctx, &end, &cls, &con, &tag) == 0) {
> +		ksmbd_debug(AUTH, "Error decoding 2nd part of negTokenInit\n");
> +		return 0;
> +	} else if ((cls != ASN1_CTX) || (con != ASN1_CON)
> +		   || (tag != ASN1_EOC)) {
> +		ksmbd_debug(AUTH,
> +			"cls = %d con = %d tag = %d end = %p (%d) exit 0\n",
> +			cls, con, tag, end, *end);
> +		return 0;
> +	}
> +
> +	/* sequence of */
> +	if (asn1_header_decode
> +	    (&ctx, &sequence_end, &cls, &con, &tag) == 0) {


I just ran checkpatch.pl on your patch and I see that you actually fixed
all the normal checkpatch.pl warnings.  But I'm used to checkpatch.pl
--strict code because that's the default in net and staging.  This file
has 1249 little things like this where checkpatch would have said to
write it like:

	if (!asn1_header_decode(&ctx, &sequence_end, &cls, &con, &tag)) {

total: 1 errors, 1 warnings, 1249 checks, 24501 lines checked

Once a patch has over a thousand style issues then it's too much for
me to handle.  :P

regards,
dan carpenter

Powered by blists - more mailing lists