mbox series

[0/5] cifsd: introduce new SMB3 kernel server

Message ID 20210322051344.1706-1-namjae.jeon@samsung.com (mailing list archive)
Headers show
Series cifsd: introduce new SMB3 kernel server | expand

Message

Namjae Jeon March 22, 2021, 5:13 a.m. UTC
This is the patch series for cifsd(ksmbd) kernel server.

What is cifsd(ksmbd) ?
======================

The SMB family of protocols is the most widely deployed
network filesystem protocol, the default on Windows and Macs (and even
on many phones and tablets), with clients and servers on all major
operating systems, but lacked a kernel server for Linux. For many
cases the current userspace server choices were suboptimal
either due to memory footprint, performance or difficulty integrating
well with advanced Linux features.

ksmbd is a new kernel module which implements the server-side of the SMB3 protocol.
The target is to provide optimized performance, GPLv2 SMB server, better
lease handling (distributed caching). The bigger goal is to add new
features more rapidly (e.g. RDMA aka "smbdirect", and recent encryption
and signing improvements to the protocol) which are easier to develop
on a smaller, more tightly optimized kernel server than for example
in Samba.  The Samba project is much broader in scope (tools, security services,
LDAP, Active Directory Domain Controller, and a cross platform file server
for a wider variety of purposes) but the user space file server portion
of Samba has proved hard to optimize for some Linux workloads, including
for smaller devices. This is not meant to replace Samba, but rather be
an extension to allow better optimizing for Linux, and will continue to
integrate well with Samba user space tools and libraries where appropriate.
Working with the Samba team we have already made sure that the configuration
files and xattrs are in a compatible format between the kernel and
user space server.


Architecture
============

               |--- ...
       --------|--- ksmbd/3 - Client 3
       |-------|--- ksmbd/2 - Client 2
       |       |         ____________________________________________________
       |       |        |- Client 1                                          |
<--- Socket ---|--- ksmbd/1   <<= Authentication : NTLM/NTLM2, Kerberos      |
       |       |      | |     <<= SMB engine : SMB2, SMB2.1, SMB3, SMB3.0.2, |
       |       |      | |                SMB3.1.1                            |
       |       |      | |____________________________________________________|
       |       |      |
       |       |      |--- VFS --- Local Filesystem
       |       |
KERNEL |--- ksmbd/0(forker kthread)
---------------||---------------------------------------------------------------
USER           ||
               || communication using NETLINK
               ||  ______________________________________________
               || |                                              |
        ksmbd.mountd <<= DCE/RPC(srvsvc, wkssvc, samr, lsarpc)   |
               ^  |  <<= configure shares setting, user accounts |
               |  |______________________________________________|
               |
               |------ smb.conf(config file)
               |
               |------ ksmbdpwd.db(user account/password file)
                            ^
  ksmbd.adduser ------------|

The subset of performance related operations(open/read/write/close etc.) belong
in kernelspace(ksmbd) and the other subset which belong to operations(DCE/RPC,
user account/share database) which are not really related with performance are
handled in userspace(ksmbd.mountd).

When the ksmbd.mountd is started, It starts up a forker thread at initialization
time and opens a dedicated port 445 for listening to SMB requests. Whenever new
clients make request, Forker thread will accept the client connection and fork
a new thread for dedicated communication channel between the client and
the server.


ksmbd feature status
====================

============================== =================================================
Feature name                   Status
============================== =================================================
Dialects                       Supported. SMB2.1 SMB3.0, SMB3.1.1 dialects
                               (intentionally excludes security vulnerable SMB1 dialect).
Auto Negotiation               Supported.
Compound Request               Supported.
Oplock Cache Mechanism         Supported.
SMB2 leases(v1 lease)          Supported.
Directory leases(v2 lease)     Planned for future.
Multi-credits                  Supported.
NTLM/NTLMv2                    Supported.
HMAC-SHA256 Signing            Supported.
Secure negotiate               Supported.
Signing Update                 Supported.
Pre-authentication integrity   Supported.
SMB3 encryption(CCM, GCM)      Supported. (CCM and GCM128 supported, GCM256 in progress)
SMB direct(RDMA)               Partially Supported. SMB3 Multi-channel is required
                               to connect to Windows client.
SMB3 Multi-channel             In Progress.
SMB3.1.1 POSIX extension       Supported.
ACLs                           Partially Supported. only DACLs available, SACLs
                               (auditing) is planned for the future. For
                               ownership (SIDs) ksmbd generates random subauth
                               values(then store it to disk) and use uid/gid
                               get from inode as RID for local domain SID.
                               The current acl implementation is limited to
                               standalone server, not a domain member.
                               Integration with Samba tools is being worked on to
                               allow future support for running as a domain member.
Kerberos                       Supported.
Durable handle v1,v2           Planned for future.
Persistent handle              Planned for future.
SMB2 notify                    Planned for future.
Sparse file support            Supported.
DCE/RPC support                Partially Supported. a few calls(NetShareEnumAll,
                               NetServerGetInfo, SAMR, LSARPC) that are needed 
                               for file server handled via netlink interface from
                               ksmbd.mountd. Additional integration with Samba
                               tools and libraries via upcall is being investigated
                               to allow support for additional DCE/RPC management
                               calls (and future support for Witness protocol e.g.)
============================== =================================================

All features required as file server are currently implemented in ksmbd.
In particular, the implementation of SMB Direct(RDMA) is only currently
possible with ksmbd (among Linux servers)


Stability
=========

It has been proved to be stable. A significant amount of xfstests pass and
are run regularly from Linux to Linux:

  http://smb3-test-rhel-75.southcentralus.cloudapp.azure.com/#/builders/8/builds/26

In addition regression tests using the broadest SMB3 functional test suite
(Samba's "smbtorture") are run on every checkin. 
It has already been used by many other open source toolkits and commercial companies
that need NAS functionality. Their issues have been fixed and contributions are
applied into ksmbd. Ksmbd has been well tested and verified in the field and market.


Mailing list and repositories
=============================
 - linux-cifsd-devel@lists.sourceforge.net
 - https://github.com/smfrench/smb3-kernel/tree/cifsd-for-next
 - https://github.com/cifsd-team/cifsd (out-of-tree)
 - https://github.com/cifsd-team/ksmbd-tools


How to run ksmbd 
================

   a. Download ksmbd-tools and compile them.
	- https://github.com/cifsd-team/ksmbd-tools

   b. Create user/password for SMB share.

	# mkdir /etc/ksmbd/
	# ksmbd.adduser -a <Enter USERNAME for SMB share access>

   c. Create /etc/ksmbd/smb.conf file, add SMB share in smb.conf file
	- Refer smb.conf.example and Documentation/configuration.txt
	  in ksmbd-tools

   d. Insert ksmbd.ko module

	# insmod ksmbd.ko

   e. Start ksmbd user space daemon
	# ksmbd.mountd

   f. Access share from Windows or Linux using SMB 
       e.g. "mount -t cifs //server/share /mnt ..."

v0:
 - fix a handful of spelling mistakes (Colin Ian King)
 - fix a precedence bug in parse_dacl() (Dan Carpenter)
 - fix a IS_ERR() vs NULL bug (Dan Carpenter)
 - fix a use after free on error path  (Dan Carpenter)
 - update cifsd.rst Documentation
 - remove unneeded FIXME comments
 - fix static checker warnings (Dan Carpenter)
 - fix WARNING: unmet direct dependencies detected for CRYPTO_ARC4 (Randy Dunlap)
 - uniquify extract_sharename() (Stephen Rothwell)
 - fix WARNING: document isn't included in any toctree (Stephen Rothwell)
 - fix WARNING: Title overline too short (Stephen Rothwell)
 - fix incorrect function comments

Namjae Jeon (5):
  cifsd: add server handler and tranport layers
  cifsd: add server-side procedures for SMB3
  cifsd: add file operations
  cifsd: add Kconfig and Makefile
  MAINTAINERS: add cifsd kernel server

 Documentation/filesystems/cifs/cifsd.rst |  180 +
 Documentation/filesystems/cifs/index.rst |   10 +
 Documentation/filesystems/index.rst      |    2 +-
 MAINTAINERS                              |   12 +-
 fs/Kconfig                               |    1 +
 fs/Makefile                              |    1 +
 fs/cifsd/Kconfig                         |   64 +
 fs/cifsd/Makefile                        |   13 +
 fs/cifsd/asn1.c                          |  702 ++
 fs/cifsd/asn1.h                          |   29 +
 fs/cifsd/auth.c                          | 1348 ++++
 fs/cifsd/auth.h                          |   90 +
 fs/cifsd/buffer_pool.c                   |  292 +
 fs/cifsd/buffer_pool.h                   |   28 +
 fs/cifsd/connection.c                    |  416 ++
 fs/cifsd/connection.h                    |  212 +
 fs/cifsd/crypto_ctx.c                    |  287 +
 fs/cifsd/crypto_ctx.h                    |   77 +
 fs/cifsd/glob.h                          |   67 +
 fs/cifsd/ksmbd_server.h                  |  285 +
 fs/cifsd/ksmbd_work.c                    |   93 +
 fs/cifsd/ksmbd_work.h                    |  124 +
 fs/cifsd/mgmt/ksmbd_ida.c                |   69 +
 fs/cifsd/mgmt/ksmbd_ida.h                |   41 +
 fs/cifsd/mgmt/share_config.c             |  238 +
 fs/cifsd/mgmt/share_config.h             |   81 +
 fs/cifsd/mgmt/tree_connect.c             |  128 +
 fs/cifsd/mgmt/tree_connect.h             |   56 +
 fs/cifsd/mgmt/user_config.c              |   69 +
 fs/cifsd/mgmt/user_config.h              |   66 +
 fs/cifsd/mgmt/user_session.c             |  344 +
 fs/cifsd/mgmt/user_session.h             |  105 +
 fs/cifsd/misc.c                          |  296 +
 fs/cifsd/misc.h                          |   38 +
 fs/cifsd/ndr.c                           |  344 +
 fs/cifsd/ndr.h                           |   21 +
 fs/cifsd/netmisc.c                       |   46 +
 fs/cifsd/nterr.c                         |  674 ++
 fs/cifsd/nterr.h                         |  552 ++
 fs/cifsd/ntlmssp.h                       |  169 +
 fs/cifsd/oplock.c                        | 1681 +++++
 fs/cifsd/oplock.h                        |  138 +
 fs/cifsd/server.c                        |  632 ++
 fs/cifsd/server.h                        |   62 +
 fs/cifsd/smb2misc.c                      |  458 ++
 fs/cifsd/smb2ops.c                       |  300 +
 fs/cifsd/smb2pdu.c                       | 8452 ++++++++++++++++++++++
 fs/cifsd/smb2pdu.h                       | 1649 +++++
 fs/cifsd/smb_common.c                    |  667 ++
 fs/cifsd/smb_common.h                    |  546 ++
 fs/cifsd/smbacl.c                        | 1324 ++++
 fs/cifsd/smbacl.h                        |  202 +
 fs/cifsd/smberr.h                        |  235 +
 fs/cifsd/smbfsctl.h                      |   90 +
 fs/cifsd/smbstatus.h                     | 1822 +++++
 fs/cifsd/time_wrappers.h                 |   34 +
 fs/cifsd/transport_ipc.c                 |  897 +++
 fs/cifsd/transport_ipc.h                 |   62 +
 fs/cifsd/transport_rdma.c                | 2051 ++++++
 fs/cifsd/transport_rdma.h                |   61 +
 fs/cifsd/transport_tcp.c                 |  625 ++
 fs/cifsd/transport_tcp.h                 |   13 +
 fs/cifsd/unicode.c                       |  391 +
 fs/cifsd/unicode.h                       |  374 +
 fs/cifsd/uniupr.h                        |  268 +
 fs/cifsd/vfs.c                           | 1989 +++++
 fs/cifsd/vfs.h                           |  314 +
 fs/cifsd/vfs_cache.c                     |  851 +++
 fs/cifsd/vfs_cache.h                     |  213 +
 69 files changed, 34069 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/filesystems/cifs/cifsd.rst
 create mode 100644 Documentation/filesystems/cifs/index.rst
 create mode 100644 fs/cifsd/Kconfig
 create mode 100644 fs/cifsd/Makefile
 create mode 100644 fs/cifsd/asn1.c
 create mode 100644 fs/cifsd/asn1.h
 create mode 100644 fs/cifsd/auth.c
 create mode 100644 fs/cifsd/auth.h
 create mode 100644 fs/cifsd/buffer_pool.c
 create mode 100644 fs/cifsd/buffer_pool.h
 create mode 100644 fs/cifsd/connection.c
 create mode 100644 fs/cifsd/connection.h
 create mode 100644 fs/cifsd/crypto_ctx.c
 create mode 100644 fs/cifsd/crypto_ctx.h
 create mode 100644 fs/cifsd/glob.h
 create mode 100644 fs/cifsd/ksmbd_server.h
 create mode 100644 fs/cifsd/ksmbd_work.c
 create mode 100644 fs/cifsd/ksmbd_work.h
 create mode 100644 fs/cifsd/mgmt/ksmbd_ida.c
 create mode 100644 fs/cifsd/mgmt/ksmbd_ida.h
 create mode 100644 fs/cifsd/mgmt/share_config.c
 create mode 100644 fs/cifsd/mgmt/share_config.h
 create mode 100644 fs/cifsd/mgmt/tree_connect.c
 create mode 100644 fs/cifsd/mgmt/tree_connect.h
 create mode 100644 fs/cifsd/mgmt/user_config.c
 create mode 100644 fs/cifsd/mgmt/user_config.h
 create mode 100644 fs/cifsd/mgmt/user_session.c
 create mode 100644 fs/cifsd/mgmt/user_session.h
 create mode 100644 fs/cifsd/misc.c
 create mode 100644 fs/cifsd/misc.h
 create mode 100644 fs/cifsd/ndr.c
 create mode 100644 fs/cifsd/ndr.h
 create mode 100644 fs/cifsd/netmisc.c
 create mode 100644 fs/cifsd/nterr.c
 create mode 100644 fs/cifsd/nterr.h
 create mode 100644 fs/cifsd/ntlmssp.h
 create mode 100644 fs/cifsd/oplock.c
 create mode 100644 fs/cifsd/oplock.h
 create mode 100644 fs/cifsd/server.c
 create mode 100644 fs/cifsd/server.h
 create mode 100644 fs/cifsd/smb2misc.c
 create mode 100644 fs/cifsd/smb2ops.c
 create mode 100644 fs/cifsd/smb2pdu.c
 create mode 100644 fs/cifsd/smb2pdu.h
 create mode 100644 fs/cifsd/smb_common.c
 create mode 100644 fs/cifsd/smb_common.h
 create mode 100644 fs/cifsd/smbacl.c
 create mode 100644 fs/cifsd/smbacl.h
 create mode 100644 fs/cifsd/smberr.h
 create mode 100644 fs/cifsd/smbfsctl.h
 create mode 100644 fs/cifsd/smbstatus.h
 create mode 100644 fs/cifsd/time_wrappers.h
 create mode 100644 fs/cifsd/transport_ipc.c
 create mode 100644 fs/cifsd/transport_ipc.h
 create mode 100644 fs/cifsd/transport_rdma.c
 create mode 100644 fs/cifsd/transport_rdma.h
 create mode 100644 fs/cifsd/transport_tcp.c
 create mode 100644 fs/cifsd/transport_tcp.h
 create mode 100644 fs/cifsd/unicode.c
 create mode 100644 fs/cifsd/unicode.h
 create mode 100644 fs/cifsd/uniupr.h
 create mode 100644 fs/cifsd/vfs.c
 create mode 100644 fs/cifsd/vfs.h
 create mode 100644 fs/cifsd/vfs_cache.c
 create mode 100644 fs/cifsd/vfs_cache.h

Comments

Dan Carpenter March 22, 2021, 6:47 a.m. UTC | #1
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
Christoph Hellwig March 22, 2021, 6:50 a.m. UTC | #2
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.
Matthew Wilcox (Oracle) March 22, 2021, 8:34 a.m. UTC | #3
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.
Sergey Senozhatsky March 22, 2021, 10:27 a.m. UTC | #4
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.
Matthew Wilcox (Oracle) March 22, 2021, 1:12 p.m. UTC | #5
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.
Stefan Metzmacher March 22, 2021, 1:25 p.m. UTC | #6
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
Namjae Jeon March 22, 2021, 11:17 p.m. UTC | #7
> 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
Namjae Jeon March 22, 2021, 11:20 p.m. UTC | #8
> 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!
Dan Carpenter March 23, 2021, 7:19 a.m. UTC | #9
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
Sebastian Gottschall March 25, 2021, 5:25 a.m. UTC | #10
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
>
>
>