Message ID | 20210322051344.1706-1-namjae.jeon@samsung.com (mailing list archive) |
---|---|
Headers | show |
Series | cifsd: introduce new SMB3 kernel server | expand |
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
On Mon, Mar 22, 2021 at 09:47:13AM +0300, Dan Carpenter wrote: > 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. > More importantly don't add another ANS1 parser, but use the generic one in lib/asn1_decoder.c instead. CIFS should also really use it.
On Mon, Mar 22, 2021 at 02:13:41PM +0900, Namjae Jeon wrote: > +++ b/fs/cifsd/mgmt/ksmbd_ida.c > @@ -0,0 +1,69 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later > +/* > + * Copyright (C) 2018 Samsung Electronics Co., Ltd. > + */ > + > +#include "ksmbd_ida.h" > + > +struct ksmbd_ida *ksmbd_ida_alloc(void) > +{ > + struct ksmbd_ida *ida; > + > + ida = kmalloc(sizeof(struct ksmbd_ida), GFP_KERNEL); > + if (!ida) > + return NULL; > + > + ida_init(&ida->map); > + return ida; > +} ... why? Everywhere that you call ksmbd_ida_alloc(), you would be better off just embedding the struct ida into the struct that currently has a pointer to it. Or declaring it statically. Then you can even initialise it statically using DEFINE_IDA() and eliminate the initialiser functions. I'd remove the ksmbd_ida abstraction, although I like this wrapper: > +int ksmbd_acquire_smb2_tid(struct ksmbd_ida *ida) > +{ > + int id; > + > + do { > + id = __acquire_id(ida, 0, 0); > + } while (id == 0xFFFF); > + > + return id; Very clever, given your constraint. I might do it as: int id = ida_alloc(ida, GFP_KERNEL); if (id == 0xffff) id = ida_alloc(ida, GFP_KERNEL); return id; Although ... > + tree_conn = ksmbd_alloc(sizeof(struct ksmbd_tree_connect)); > + if (!tree_conn) { > + status.ret = -ENOMEM; > + goto out_error; > + } > + > + tree_conn->id = ksmbd_acquire_tree_conn_id(sess); > + if (tree_conn->id < 0) { > + status.ret = -EINVAL; > + goto out_error; > + } > + > + peer_addr = KSMBD_TCP_PEER_SOCKADDR(sess->conn); > + resp = ksmbd_ipc_tree_connect_request(sess, > + sc, > + tree_conn, > + peer_addr); > + if (!resp) { > + status.ret = -EINVAL; > + goto out_error; > + } > + > + status.ret = resp->status; > + if (status.ret != KSMBD_TREE_CONN_STATUS_OK) > + goto out_error; > + > + tree_conn->flags = resp->connection_flags; > + tree_conn->user = sess->user; > + tree_conn->share_conf = sc; > + status.tree_conn = tree_conn; > + > + list_add(&tree_conn->list, &sess->tree_conn_list); This is basically the only function which calls that, and this is a relatively common anti-pattern when using the IDA -- you've allocated a unique ID, but then you stuff the object in a list and ... > +struct ksmbd_tree_connect *ksmbd_tree_conn_lookup(struct ksmbd_session *sess, > + unsigned int id) > +{ > + struct ksmbd_tree_connect *tree_conn; > + struct list_head *tmp; > + > + list_for_each(tmp, &sess->tree_conn_list) { > + tree_conn = list_entry(tmp, struct ksmbd_tree_connect, list); > + if (tree_conn->id == id) > + return tree_conn; > + } ... walk the linked list looking for an ID match. You'd be much better off using an allocating XArray: https://www.kernel.org/doc/html/latest/core-api/xarray.html Then you could lookup tree connections in O(log(n)) time instead of O(n) time.
On (21/03/22 08:34), Matthew Wilcox wrote: > > +++ b/fs/cifsd/mgmt/ksmbd_ida.c > > @@ -0,0 +1,69 @@ > > +// SPDX-License-Identifier: GPL-2.0-or-later > > +/* > > + * Copyright (C) 2018 Samsung Electronics Co., Ltd. > > + */ > > + > > +#include "ksmbd_ida.h" > > + > > +struct ksmbd_ida *ksmbd_ida_alloc(void) > > +{ > > + struct ksmbd_ida *ida; > > + > > + ida = kmalloc(sizeof(struct ksmbd_ida), GFP_KERNEL); > > + if (!ida) > > + return NULL; > > + > > + ida_init(&ida->map); > > + return ida; > > +} > > ... why? Everywhere that you call ksmbd_ida_alloc(), you would > be better off just embedding the struct ida into the struct that > currently has a pointer to it. Or declaring it statically. Then > you can even initialise it statically using DEFINE_IDA() and > eliminate the initialiser functions. IIRC this ida is per SMB session, so it probably cannot be static. And Windows, IIRC, doesn't like "just any IDs". Some versions of Windows would fail the session login if server would return the first id == 0, instead of 1. Or vice versa. I don't remember all the details, the last time I looked into this was in 2019. [..] > > +struct ksmbd_tree_connect *ksmbd_tree_conn_lookup(struct ksmbd_session *sess, > > + unsigned int id) > > +{ > > + struct ksmbd_tree_connect *tree_conn; > > + struct list_head *tmp; > > + > > + list_for_each(tmp, &sess->tree_conn_list) { > > + tree_conn = list_entry(tmp, struct ksmbd_tree_connect, list); > > + if (tree_conn->id == id) > > + return tree_conn; > > + } > > ... walk the linked list looking for an ID match. You'd be much better > off using an allocating XArray: > https://www.kernel.org/doc/html/latest/core-api/xarray.html I think cifsd code predates XArray ;) > Then you could lookup tree connections in O(log(n)) time instead of > O(n) time. Agreed. Not sure I remember why the code does list traversal here.
On Mon, Mar 22, 2021 at 07:27:03PM +0900, Sergey Senozhatsky wrote: > On (21/03/22 08:34), Matthew Wilcox wrote: > > > +++ b/fs/cifsd/mgmt/ksmbd_ida.c > > > @@ -0,0 +1,69 @@ > > > +// SPDX-License-Identifier: GPL-2.0-or-later > > > +/* > > > + * Copyright (C) 2018 Samsung Electronics Co., Ltd. > > > + */ > > > + > > > +#include "ksmbd_ida.h" > > > + > > > +struct ksmbd_ida *ksmbd_ida_alloc(void) > > > +{ > > > + struct ksmbd_ida *ida; > > > + > > > + ida = kmalloc(sizeof(struct ksmbd_ida), GFP_KERNEL); > > > + if (!ida) > > > + return NULL; > > > + > > > + ida_init(&ida->map); > > > + return ida; > > > +} > > > > ... why? Everywhere that you call ksmbd_ida_alloc(), you would > > be better off just embedding the struct ida into the struct that > > currently has a pointer to it. Or declaring it statically. Then > > you can even initialise it statically using DEFINE_IDA() and > > eliminate the initialiser functions. > > IIRC this ida is per SMB session, so it probably cannot be static. Depends which IDA you're talking about. +struct ksmbd_conn *ksmbd_conn_alloc(void) + conn->async_ida = ksmbd_ida_alloc(); Embed into 'conn'. +static struct ksmbd_ida *ida; +int ksmbd_ipc_init(void) + ida = ksmbd_ida_alloc(); Should be static. > And Windows, IIRC, doesn't like "just any IDs". Some versions of Windows > would fail the session login if server would return the first id == 0, > instead of 1. Or vice versa. I don't remember all the details, the last > time I looked into this was in 2019. Sure, you can keep that logic. > > ... walk the linked list looking for an ID match. You'd be much better > > off using an allocating XArray: > > https://www.kernel.org/doc/html/latest/core-api/xarray.html > > I think cifsd code predates XArray ;) Sure, but you could have used an IDR ;-) > > Then you could lookup tree connections in O(log(n)) time instead of > > O(n) time. > > Agreed. Not sure I remember why the code does list traversal here.
Am 22.03.21 um 07:50 schrieb Christoph Hellwig: > On Mon, Mar 22, 2021 at 09:47:13AM +0300, Dan Carpenter wrote: >> 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. >> > > More importantly don't add another ANS1 parser, but use the generic > one in lib/asn1_decoder.c instead. CIFS should also really use it. I think the best would be to avoid asn1 completely in the kernel and do the whole authentication in userspace. The kernel can only deal this blobs here, I don't there's need to look inside the blobs. 1. ksmbd-mount would provide a fixed initial blob that's always the same and will be returned in the "2.2.4 SMB2 NEGOTIATE Response" PDU as SecurityBuffer 2. The kernel just blindly forwards the SecurityBuffer of "2.2.5 SMB2 SESSION_SETUP Request" to userspace together with the client provided SessionId (from 2.2.1.2 SMB2 Packet Header - SYNC) as well as negotiated signing and encryption algorithm ids and the latest preauth hash. 3. Userspace passes a NTSTATUS together with SecurityBuffer blob for the 2.2.6 SMB2 SESSION_SETUP Response back to the kernel: - NT_STATUS_MORE_PROCESSING_REQUIRED (more authentication legs are required) SecurityBuffer is most likely a non empty buffer - NT_STATUS_OK - The authentication is complete: SecurityBuffer might be empty or not It also pass a channel signing key, a decryption and encrytion key as well as the unix token ( I guess in the current form it's only uid/gid) down to the kernel - Any other status means the authentication failed, which is a hard error for the client The PDU definitions are defined here: https://docs.microsoft.com/en-us/openspecs/windows_protocols/ms-smb2/6eaf6e75-9c23-4eda-be99-c9223c60b181 I think everything else belongs to userspace. Such a "simple" design for the kernel part, would mean that ksmbd-mount would do what the kernel part is currently doing, but it also means it will be trivial to plug the userspace part to samba's winbindd in future order to get domain wide authentication. metze
> 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
> On Mon, Mar 22, 2021 at 09:47:13AM +0300, Dan Carpenter wrote: > > 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. > > > > More importantly don't add another ANS1 parser, but use the generic one in lib/asn1_decoder.c instead. > CIFS should also really use it. Okay, Let me check it, cifs also... Thanks!
On Tue, Mar 23, 2021 at 08:17:47AM +0900, Namjae Jeon wrote: > > > + > > > +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... > Boolean functions should tell you what they are testing in the name. Without any context you can't know what if (compare_oid(one, two)) { means, but if (oid_equal(one, two)) { is readable. regards, dan carpenter
Am 23.03.2021 um 08:19 schrieb Dan Carpenter: > On Tue, Mar 23, 2021 at 08:17:47AM +0900, Namjae Jeon wrote: >>>> + >>>> +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... >> > Boolean functions should tell you what they are testing in the name. > Without any context you can't know what if (compare_oid(one, two)) { > means, but if (oid_equal(one, two)) { is readable. > > regards, > dan carpenter ahm just a pointless comment. but return !memcmp(oid1,oid2, sizeof(long*)*oid1len); looks much more efficient than this "for" loop > > >