Message-ID: <009b01d71f71$9224f4e0$b66edea0$@samsung.com>
Date:   Tue, 23 Mar 2021 08:17:47 +0900
From:   "Namjae Jeon" <namjae.jeon@...sung.com>
To:     "'Dan Carpenter'" <dan.carpenter@...cle.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.
Okay.
> 
> > +
> > +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.
Okay.
> 
> > +
> > +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.
Okay.
> 
> > +
> > +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.
Okay.
> 
> > +
> > +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.
Okay.
> 
> > +
> > +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()?
Why not compare_oid()? This code is come from cifs.
I need clear reason to change both cifs/cifsd...

> 
> 
> > +
> > +/* 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.
Okay.
> 
> 	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
Okay, I'll run it with that option:)

Thanks for your review!
> 
> regards,
> dan carpenter

Powered by blists - more mailing lists