| 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
| ||
|
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