Revision 49054556289e8787501630b7c7a9d407da02e296 authored by Paolo Abeni on 29 September 2021, 09:59:17 UTC, committed by David S. Miller on 30 September 2021, 12:06:47 UTC
Syzkaller reported a false positive deadlock involving
the nl socket lock and the subflow socket lock:

MPTCP: kernel_bind error, err=-98
============================================
WARNING: possible recursive locking detected
5.15.0-rc1-syzkaller #0 Not tainted
--------------------------------------------
syz-executor998/6520 is trying to acquire lock:
ffff8880795718a0 (k-sk_lock-AF_INET){+.+.}-{0:0}, at: mptcp_close+0x267/0x7b0 net/mptcp/protocol.c:2738

but task is already holding lock:
ffff8880787c8c60 (k-sk_lock-AF_INET){+.+.}-{0:0}, at: lock_sock include/net/sock.h:1612 [inline]
ffff8880787c8c60 (k-sk_lock-AF_INET){+.+.}-{0:0}, at: mptcp_close+0x23/0x7b0 net/mptcp/protocol.c:2720

other info that might help us debug this:
 Possible unsafe locking scenario:

       CPU0
       ----
  lock(k-sk_lock-AF_INET);
  lock(k-sk_lock-AF_INET);

 *** DEADLOCK ***

 May be due to missing lock nesting notation

3 locks held by syz-executor998/6520:
 #0: ffffffff8d176c50 (cb_lock){++++}-{3:3}, at: genl_rcv+0x15/0x40 net/netlink/genetlink.c:802
 #1: ffffffff8d176d08 (genl_mutex){+.+.}-{3:3}, at: genl_lock net/netlink/genetlink.c:33 [inline]
 #1: ffffffff8d176d08 (genl_mutex){+.+.}-{3:3}, at: genl_rcv_msg+0x3e0/0x580 net/netlink/genetlink.c:790
 #2: ffff8880787c8c60 (k-sk_lock-AF_INET){+.+.}-{0:0}, at: lock_sock include/net/sock.h:1612 [inline]
 #2: ffff8880787c8c60 (k-sk_lock-AF_INET){+.+.}-{0:0}, at: mptcp_close+0x23/0x7b0 net/mptcp/protocol.c:2720

stack backtrace:
CPU: 1 PID: 6520 Comm: syz-executor998 Not tainted 5.15.0-rc1-syzkaller #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
Call Trace:
 __dump_stack lib/dump_stack.c:88 [inline]
 dump_stack_lvl+0xcd/0x134 lib/dump_stack.c:106
 print_deadlock_bug kernel/locking/lockdep.c:2944 [inline]
 check_deadlock kernel/locking/lockdep.c:2987 [inline]
 validate_chain kernel/locking/lockdep.c:3776 [inline]
 __lock_acquire.cold+0x149/0x3ab kernel/locking/lockdep.c:5015
 lock_acquire kernel/locking/lockdep.c:5625 [inline]
 lock_acquire+0x1ab/0x510 kernel/locking/lockdep.c:5590
 lock_sock_fast+0x36/0x100 net/core/sock.c:3229
 mptcp_close+0x267/0x7b0 net/mptcp/protocol.c:2738
 inet_release+0x12e/0x280 net/ipv4/af_inet.c:431
 __sock_release net/socket.c:649 [inline]
 sock_release+0x87/0x1b0 net/socket.c:677
 mptcp_pm_nl_create_listen_socket+0x238/0x2c0 net/mptcp/pm_netlink.c:900
 mptcp_nl_cmd_add_addr+0x359/0x930 net/mptcp/pm_netlink.c:1170
 genl_family_rcv_msg_doit+0x228/0x320 net/netlink/genetlink.c:731
 genl_family_rcv_msg net/netlink/genetlink.c:775 [inline]
 genl_rcv_msg+0x328/0x580 net/netlink/genetlink.c:792
 netlink_rcv_skb+0x153/0x420 net/netlink/af_netlink.c:2504
 genl_rcv+0x24/0x40 net/netlink/genetlink.c:803
 netlink_unicast_kernel net/netlink/af_netlink.c:1314 [inline]
 netlink_unicast+0x533/0x7d0 net/netlink/af_netlink.c:1340
 netlink_sendmsg+0x86d/0xdb0 net/netlink/af_netlink.c:1929
 sock_sendmsg_nosec net/socket.c:704 [inline]
 sock_sendmsg+0xcf/0x120 net/socket.c:724
 sock_no_sendpage+0x101/0x150 net/core/sock.c:2980
 kernel_sendpage.part.0+0x1a0/0x340 net/socket.c:3504
 kernel_sendpage net/socket.c:3501 [inline]
 sock_sendpage+0xe5/0x140 net/socket.c:1003
 pipe_to_sendpage+0x2ad/0x380 fs/splice.c:364
 splice_from_pipe_feed fs/splice.c:418 [inline]
 __splice_from_pipe+0x43e/0x8a0 fs/splice.c:562
 splice_from_pipe fs/splice.c:597 [inline]
 generic_splice_sendpage+0xd4/0x140 fs/splice.c:746
 do_splice_from fs/splice.c:767 [inline]
 direct_splice_actor+0x110/0x180 fs/splice.c:936
 splice_direct_to_actor+0x34b/0x8c0 fs/splice.c:891
 do_splice_direct+0x1b3/0x280 fs/splice.c:979
 do_sendfile+0xae9/0x1240 fs/read_write.c:1249
 __do_sys_sendfile64 fs/read_write.c:1314 [inline]
 __se_sys_sendfile64 fs/read_write.c:1300 [inline]
 __x64_sys_sendfile64+0x1cc/0x210 fs/read_write.c:1300
 do_syscall_x64 arch/x86/entry/common.c:50 [inline]
 do_syscall_64+0x35/0xb0 arch/x86/entry/common.c:80
 entry_SYSCALL_64_after_hwframe+0x44/0xae
RIP: 0033:0x7f215cb69969
Code: 28 00 00 00 75 05 48 83 c4 28 c3 e8 e1 14 00 00 90 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 c0 ff ff ff f7 d8 64 89 01 48
RSP: 002b:00007ffc96bb3868 EFLAGS: 00000246 ORIG_RAX: 0000000000000028
RAX: ffffffffffffffda RBX: 00007f215cbad072 RCX: 00007f215cb69969
RDX: 0000000000000000 RSI: 0000000000000004 RDI: 0000000000000005
RBP: 0000000000000000 R08: 00007ffc96bb3a08 R09: 00007ffc96bb3a08
R10: 0000000100000002 R11: 0000000000000246 R12: 00007ffc96bb387c
R13: 431bde82d7b634db R14: 0000000000000000 R15: 0000000000000000

the problem originates from uncorrect lock annotation in the mptcp
code and is only visible since commit 2dcb96bacce3 ("net: core: Correct
the sock::sk_lock.owned lockdep annotations"), but is present since
the port-based endpoint support initial implementation.

This patch addresses the issue introducing a nested variant of
lock_sock_fast() and using it in the relevant code path.

Fixes: 1729cf186d8a ("mptcp: create the listening socket for new port")
Fixes: 2dcb96bacce3 ("net: core: Correct the sock::sk_lock.owned lockdep annotations")
Suggested-by: Thomas Gleixner <tglx@linutronix.de>
Reported-and-tested-by: syzbot+1dd53f7a89b299d59eaf@syzkaller.appspotmail.com
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: David S. Miller <davem@davemloft.net>
1 parent d88fd1b
Raw File
asn1_encoder.c
// SPDX-License-Identifier: GPL-2.0-only
/*
 * Simple encoder primitives for ASN.1 BER/DER/CER
 *
 * Copyright (C) 2019 James.Bottomley@HansenPartnership.com
 */

#include <linux/asn1_encoder.h>
#include <linux/bug.h>
#include <linux/string.h>
#include <linux/module.h>

/**
 * asn1_encode_integer() - encode positive integer to ASN.1
 * @data:	pointer to the pointer to the data
 * @end_data:	end of data pointer, points one beyond last usable byte in @data
 * @integer:	integer to be encoded
 *
 * This is a simplified encoder: it only currently does
 * positive integers, but it should be simple enough to add the
 * negative case if a use comes along.
 */
unsigned char *
asn1_encode_integer(unsigned char *data, const unsigned char *end_data,
		    s64 integer)
{
	int data_len = end_data - data;
	unsigned char *d = &data[2];
	bool found = false;
	int i;

	if (WARN(integer < 0,
		 "BUG: integer encode only supports positive integers"))
		return ERR_PTR(-EINVAL);

	if (IS_ERR(data))
		return data;

	/* need at least 3 bytes for tag, length and integer encoding */
	if (data_len < 3)
		return ERR_PTR(-EINVAL);

	/* remaining length where at d (the start of the integer encoding) */
	data_len -= 2;

	data[0] = _tag(UNIV, PRIM, INT);
	if (integer == 0) {
		*d++ = 0;
		goto out;
	}

	for (i = sizeof(integer); i > 0 ; i--) {
		int byte = integer >> (8 * (i - 1));

		if (!found && byte == 0)
			continue;

		/*
		 * for a positive number the first byte must have bit
		 * 7 clear in two's complement (otherwise it's a
		 * negative number) so prepend a leading zero if
		 * that's not the case
		 */
		if (!found && (byte & 0x80)) {
			/*
			 * no check needed here, we already know we
			 * have len >= 1
			 */
			*d++ = 0;
			data_len--;
		}

		found = true;
		if (data_len == 0)
			return ERR_PTR(-EINVAL);

		*d++ = byte;
		data_len--;
	}

 out:
	data[1] = d - data - 2;

	return d;
}
EXPORT_SYMBOL_GPL(asn1_encode_integer);

/* calculate the base 128 digit values setting the top bit of the first octet */
static int asn1_encode_oid_digit(unsigned char **_data, int *data_len, u32 oid)
{
	unsigned char *data = *_data;
	int start = 7 + 7 + 7 + 7;
	int ret = 0;

	if (*data_len < 1)
		return -EINVAL;

	/* quick case */
	if (oid == 0) {
		*data++ = 0x80;
		(*data_len)--;
		goto out;
	}

	while (oid >> start == 0)
		start -= 7;

	while (start > 0 && *data_len > 0) {
		u8 byte;

		byte = oid >> start;
		oid = oid - (byte << start);
		start -= 7;
		byte |= 0x80;
		*data++ = byte;
		(*data_len)--;
	}

	if (*data_len > 0) {
		*data++ = oid;
		(*data_len)--;
	} else {
		ret = -EINVAL;
	}

 out:
	*_data = data;
	return ret;
}

/**
 * asn1_encode_oid() - encode an oid to ASN.1
 * @data:	position to begin encoding at
 * @end_data:	end of data pointer, points one beyond last usable byte in @data
 * @oid:	array of oids
 * @oid_len:	length of oid array
 *
 * this encodes an OID up to ASN.1 when presented as an array of OID values
 */
unsigned char *
asn1_encode_oid(unsigned char *data, const unsigned char *end_data,
		u32 oid[], int oid_len)
{
	int data_len = end_data - data;
	unsigned char *d = data + 2;
	int i, ret;

	if (WARN(oid_len < 2, "OID must have at least two elements"))
		return ERR_PTR(-EINVAL);

	if (WARN(oid_len > 32, "OID is too large"))
		return ERR_PTR(-EINVAL);

	if (IS_ERR(data))
		return data;


	/* need at least 3 bytes for tag, length and OID encoding */
	if (data_len < 3)
		return ERR_PTR(-EINVAL);

	data[0] = _tag(UNIV, PRIM, OID);
	*d++ = oid[0] * 40 + oid[1];

	data_len -= 3;

	ret = 0;

	for (i = 2; i < oid_len; i++) {
		ret = asn1_encode_oid_digit(&d, &data_len, oid[i]);
		if (ret < 0)
			return ERR_PTR(ret);
	}

	data[1] = d - data - 2;

	return d;
}
EXPORT_SYMBOL_GPL(asn1_encode_oid);

/**
 * asn1_encode_length() - encode a length to follow an ASN.1 tag
 * @data: pointer to encode at
 * @data_len: pointer to remaining length (adjusted by routine)
 * @len: length to encode
 *
 * This routine can encode lengths up to 65535 using the ASN.1 rules.
 * It will accept a negative length and place a zero length tag
 * instead (to keep the ASN.1 valid).  This convention allows other
 * encoder primitives to accept negative lengths as singalling the
 * sequence will be re-encoded when the length is known.
 */
static int asn1_encode_length(unsigned char **data, int *data_len, int len)
{
	if (*data_len < 1)
		return -EINVAL;

	if (len < 0) {
		*((*data)++) = 0;
		(*data_len)--;
		return 0;
	}

	if (len <= 0x7f) {
		*((*data)++) = len;
		(*data_len)--;
		return 0;
	}

	if (*data_len < 2)
		return -EINVAL;

	if (len <= 0xff) {
		*((*data)++) = 0x81;
		*((*data)++) = len & 0xff;
		*data_len -= 2;
		return 0;
	}

	if (*data_len < 3)
		return -EINVAL;

	if (len <= 0xffff) {
		*((*data)++) = 0x82;
		*((*data)++) = (len >> 8) & 0xff;
		*((*data)++) = len & 0xff;
		*data_len -= 3;
		return 0;
	}

	if (WARN(len > 0xffffff, "ASN.1 length can't be > 0xffffff"))
		return -EINVAL;

	if (*data_len < 4)
		return -EINVAL;
	*((*data)++) = 0x83;
	*((*data)++) = (len >> 16) & 0xff;
	*((*data)++) = (len >> 8) & 0xff;
	*((*data)++) = len & 0xff;
	*data_len -= 4;

	return 0;
}

/**
 * asn1_encode_tag() - add a tag for optional or explicit value
 * @data:	pointer to place tag at
 * @end_data:	end of data pointer, points one beyond last usable byte in @data
 * @tag:	tag to be placed
 * @string:	the data to be tagged
 * @len:	the length of the data to be tagged
 *
 * Note this currently only handles short form tags < 31.
 *
 * Standard usage is to pass in a @tag, @string and @length and the
 * @string will be ASN.1 encoded with @tag and placed into @data.  If
 * the encoding would put data past @end_data then an error is
 * returned, otherwise a pointer to a position one beyond the encoding
 * is returned.
 *
 * To encode in place pass a NULL @string and -1 for @len and the
 * maximum allowable beginning and end of the data; all this will do
 * is add the current maximum length and update the data pointer to
 * the place where the tag contents should be placed is returned.  The
 * data should be copied in by the calling routine which should then
 * repeat the prior statement but now with the known length.  In order
 * to avoid having to keep both before and after pointers, the repeat
 * expects to be called with @data pointing to where the first encode
 * returned it and still NULL for @string but the real length in @len.
 */
unsigned char *
asn1_encode_tag(unsigned char *data, const unsigned char *end_data,
		u32 tag, const unsigned char *string, int len)
{
	int data_len = end_data - data;
	int ret;

	if (WARN(tag > 30, "ASN.1 tag can't be > 30"))
		return ERR_PTR(-EINVAL);

	if (!string && WARN(len > 127,
			    "BUG: recode tag is too big (>127)"))
		return ERR_PTR(-EINVAL);

	if (IS_ERR(data))
		return data;

	if (!string && len > 0) {
		/*
		 * we're recoding, so move back to the start of the
		 * tag and install a dummy length because the real
		 * data_len should be NULL
		 */
		data -= 2;
		data_len = 2;
	}

	if (data_len < 2)
		return ERR_PTR(-EINVAL);

	*(data++) = _tagn(CONT, CONS, tag);
	data_len--;
	ret = asn1_encode_length(&data, &data_len, len);
	if (ret < 0)
		return ERR_PTR(ret);

	if (!string)
		return data;

	if (data_len < len)
		return ERR_PTR(-EINVAL);

	memcpy(data, string, len);
	data += len;

	return data;
}
EXPORT_SYMBOL_GPL(asn1_encode_tag);

/**
 * asn1_encode_octet_string() - encode an ASN.1 OCTET STRING
 * @data:	pointer to encode at
 * @end_data:	end of data pointer, points one beyond last usable byte in @data
 * @string:	string to be encoded
 * @len:	length of string
 *
 * Note ASN.1 octet strings may contain zeros, so the length is obligatory.
 */
unsigned char *
asn1_encode_octet_string(unsigned char *data,
			 const unsigned char *end_data,
			 const unsigned char *string, u32 len)
{
	int data_len = end_data - data;
	int ret;

	if (IS_ERR(data))
		return data;

	/* need minimum of 2 bytes for tag and length of zero length string */
	if (data_len < 2)
		return ERR_PTR(-EINVAL);

	*(data++) = _tag(UNIV, PRIM, OTS);
	data_len--;

	ret = asn1_encode_length(&data, &data_len, len);
	if (ret)
		return ERR_PTR(ret);

	if (data_len < len)
		return ERR_PTR(-EINVAL);

	memcpy(data, string, len);
	data += len;

	return data;
}
EXPORT_SYMBOL_GPL(asn1_encode_octet_string);

/**
 * asn1_encode_sequence() - wrap a byte stream in an ASN.1 SEQUENCE
 * @data:	pointer to encode at
 * @end_data:	end of data pointer, points one beyond last usable byte in @data
 * @seq:	data to be encoded as a sequence
 * @len:	length of the data to be encoded as a sequence
 *
 * Fill in a sequence.  To encode in place, pass NULL for @seq and -1
 * for @len; then call again once the length is known (still with NULL
 * for @seq). In order to avoid having to keep both before and after
 * pointers, the repeat expects to be called with @data pointing to
 * where the first encode placed it.
 */
unsigned char *
asn1_encode_sequence(unsigned char *data, const unsigned char *end_data,
		     const unsigned char *seq, int len)
{
	int data_len = end_data - data;
	int ret;

	if (!seq && WARN(len > 127,
			 "BUG: recode sequence is too big (>127)"))
		return ERR_PTR(-EINVAL);

	if (IS_ERR(data))
		return data;

	if (!seq && len >= 0) {
		/*
		 * we're recoding, so move back to the start of the
		 * sequence and install a dummy length because the
		 * real length should be NULL
		 */
		data -= 2;
		data_len = 2;
	}

	if (data_len < 2)
		return ERR_PTR(-EINVAL);

	*(data++) = _tag(UNIV, CONS, SEQ);
	data_len--;

	ret = asn1_encode_length(&data, &data_len, len);
	if (ret)
		return ERR_PTR(ret);

	if (!seq)
		return data;

	if (data_len < len)
		return ERR_PTR(-EINVAL);

	memcpy(data, seq, len);
	data += len;

	return data;
}
EXPORT_SYMBOL_GPL(asn1_encode_sequence);

/**
 * asn1_encode_boolean() - encode a boolean value to ASN.1
 * @data:	pointer to encode at
 * @end_data:	end of data pointer, points one beyond last usable byte in @data
 * @val:	the boolean true/false value
 */
unsigned char *
asn1_encode_boolean(unsigned char *data, const unsigned char *end_data,
		    bool val)
{
	int data_len = end_data - data;

	if (IS_ERR(data))
		return data;

	/* booleans are 3 bytes: tag, length == 1 and value == 0 or 1 */
	if (data_len < 3)
		return ERR_PTR(-EINVAL);

	*(data++) = _tag(UNIV, PRIM, BOOL);
	data_len--;

	asn1_encode_length(&data, &data_len, 1);

	if (val)
		*(data++) = 1;
	else
		*(data++) = 0;

	return data;
}
EXPORT_SYMBOL_GPL(asn1_encode_boolean);

MODULE_LICENSE("GPL");
back to top