Received-SPF: pass (google.com: domain of linux-nfs+bounces-1002-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Wed, 10 Jan 2024 12:40:41 +0800
From: Pengfei Xu <pengfei.xu@intel.com>
To: David Howells <dhowells@redhat.com>, <eadavis@qq.com>
CC: Linus Torvalds <torvalds@linux-foundation.org>, Edward Adam Davis
	<eadavis@qq.com>, Simon Horman <horms@kernel.org>, Markus Suvanto
	<markus.suvanto@gmail.com>, Jeffrey E Altman <jaltman@auristor.com>, "Marc
 Dionne" <marc.dionne@auristor.com>, Wang Lei <wang840925@gmail.com>, "Jeff
 Layton" <jlayton@redhat.com>, Steve French <smfrench@gmail.com>, "Jarkko
 Sakkinen" <jarkko@kernel.org>, "David S. Miller" <davem@davemloft.net>, "Eric
 Dumazet" <edumazet@google.com>, Jakub Kicinski <kuba@kernel.org>, Paolo Abeni
	<pabeni@redhat.com>, <linux-afs@lists.infradead.org>,
	<keyrings@vger.kernel.org>, <linux-cifs@vger.kernel.org>,
	<linux-nfs@vger.kernel.org>, <ceph-devel@vger.kernel.org>,
	<netdev@vger.kernel.org>, <linux-fsdevel@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>, <heng.su@intel.com>, <pengfei.xu@intel.com>
Subject: Re: [PATCH] keys, dns: Fix missing size check of V1 server-list
 header
Message-ID: <ZZ4fyY4r3rqgZL+4@xpf.sh.intel.com>
References: <CAHk-=wgJz36ZE66_8gXjP_TofkkugXBZEpTr_Dtc_JANsH1SEw@mail.gmail.com>
 <1843374.1703172614@warthog.procyon.org.uk>
 <20231223172858.GI201037@kernel.org>
 <2592945.1703376169@warthog.procyon.org.uk>
Content-Type: multipart/mixed; boundary="CKlVRO8OFc+GHLsw"
Content-Disposition: inline
In-Reply-To: <2592945.1703376169@warthog.procyon.org.uk>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?yMwpjSqCy1UOQ6NprMf4CYU/hCkNhTayaq1PAH8wGJJZUOQdN9JSdIxRN5P7?=
 =?us-ascii?Q?Wo0VLbKbhHyUoHPkSUjSHBWj6lQiqsglwlUnrr+8isOYtyheQqQ//NwTOvmD?=
 =?us-ascii?Q?XEoRC/vWKx9NRTK9Y3tW+IkBgQ2eOdeplL/4aPj1q40GiD/V+Jh4QJfzWQHB?=
 =?us-ascii?Q?hSPRf2BoYKXrbkjP4EELFg5tPiIh/8clTSR2ipMKRpCrKLzMhtq98Bo5/Gum?=
 =?us-ascii?Q?ivbX3SbOzf0sxspAJ5+tsj/a2jwflXa0uScK90B6fcy7HgEqCM1NGsRb6Dzt?=
 =?us-ascii?Q?ekf1L5lka+IL+OkXuCqT/hFwqmvJ2HAIJhiq5kLluOBCYG5Fq/xQjm+JRT+x?=
 =?us-ascii?Q?WXi2HE8LrnYzzI1jkD7py4ylXwjz84KwtK5ouyQfFGlpwVcc4cCZT+MqkCzB?=
 =?us-ascii?Q?CHANgWTWTzlUWQxcdrphGmZ+hc08/k44rzGAjsnropbIQyTrcfH1z+K/xbl9?=
 =?us-ascii?Q?fuBJCmmGZyp4H8XjR+ekkUbd+y2ngwfTYV+X0No38KL/UZy2BluwM8dbq2Q8?=
 =?us-ascii?Q?QZOBMEzMiGmPU9sM/yhDG29/mbX0v2rYTpRQQI2mYlDXpAD0xHoedUCsEl4+?=
 =?us-ascii?Q?8qBqzidMuT6Fv2CW2PX+hwcokwokZuoBXQnowAB1bqTaKMfNm2Bk/Q6rPmp/?=
 =?us-ascii?Q?AxoNGt2lLKPZIa3vj8dEU8nj7QvoqXDd6KdjHFZOMto0BzSnrm6XN/HaCt5i?=
 =?us-ascii?Q?KpPmN9/NWVUSvG4/Eo3bjpe9o7NugPusO46eLWohTR2/PCR0YMnaqT46tp/t?=
 =?us-ascii?Q?cbd5vMJbJg0OPDEvv4zsp3BjQUiGxNqcewib3oFZZqIOkCMV1/dGyRbILF0X?=
 =?us-ascii?Q?1BCPqcP/UDBKqoOEMoroVK/hnznC2+IZ1DCR3KqZkHN8RtIf5UcpG6RxVQso?=
 =?us-ascii?Q?QnlnZC+IfOWB71IjcmK+HdO25AH3ChJR0N75lPOSWG+gX12rDzFr3qEWTkdn?=
 =?us-ascii?Q?hvemrTkMvSSogZpIsR3h2Gn7o3LAa8ZuPTXuXIa/R1fYXZ2H26xCD0T63d6g?=
 =?us-ascii?Q?DhcQ9ZaK/TgP4WQqIysj8hd5+SYCLzJVrU0GEd+iHIyWK6gNhBoZrNPnfqbE?=
 =?us-ascii?Q?IwWbaiXAzGyfPuPfNMAogr5ZkstpNOCqdWrLUawAorcnUyFamBQrnD3qcRAc?=
 =?us-ascii?Q?2DPC/C0/WOFVvoaWsVuFbRcsx/5KkBDvT38JBSjaBLU9kiqzzy+p3zLS6yzA?=
 =?us-ascii?Q?dM6uWDbeVWB7TSMX8qmZ4r9gGM3QfbOlNiANjUUUxWUhtDwO7/NVQ1t+YSFJ?=
 =?us-ascii?Q?6vCvyz3yzlkeNYjiO/dwUSq8SuYwEWfVVJA1e3GAyvh7r/s1Afm7JuvqP74D?=
 =?us-ascii?Q?HsvotqAU7EKXkib1umHNQqZ6aUfi6t1oaunyU4olgceyOz0R2MAjhrK2eNdx?=
 =?us-ascii?Q?35ixSrtfKdUkEpKWFQPhHgSZo6SnQgN+jBwOyrrrIz8HmBlQIbH7hBO4DnGh?=
 =?us-ascii?Q?g7Xx8BBBt+Itva5nfVgzJ4U27Rq0D5kpjBHlgx48f+spiEyeylHuB4zONyee?=
 =?us-ascii?Q?2smQSKhm3VcJWhhXIQe9vjvohKU/LTsKhOUJDj08NPAnv2KSL7OUaQ5Ffyir?=
 =?us-ascii?Q?EWKqu3dFoDlDCysvLdCBUSDeSwPEvm8DtpWu/kG/?=
X-MS-Exchange-CrossTenant-Network-Message-Id: edd75696-ccdd-4b55-2651-08dc119714e6
X-MS-Exchange-CrossTenant-AuthSource: PH0PR11MB4839.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 10 Jan 2024 04:46:16.9905
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: BBY+9OO6KgMPYR5vVTGQXAEZR2fPvTR/9ZLud2kyzM6k4j7xAlL4s1fWVxZu+uSbnlOqcJRPhtAIttwnIHAv/w==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA1PR11MB6710
X-OriginatorOrg: intel.com

--CKlVRO8OFc+GHLsw
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline

On 2023-12-24 at 00:02:49 +0000, David Howells wrote:
> Hi Linus, Edward,
> 
> Here's Linus's patch dressed up with a commit message.  I would marginally
> prefer just to insert the missing size check, but I'm also fine with Linus's
> approach for now until we have different content types or newer versions.
> 
> Note that I'm not sure whether I should require Linus's S-o-b since he made
> modifications or whether I should use a Codeveloped-by line for him.
> 
> David
> ---
> From: Edward Adam Davis <eadavis@qq.com>
> 
> keys, dns: Fix missing size check of V1 server-list header
> 
> The dns_resolver_preparse() function has a check on the size of the payload
> for the basic header of the binary-style payload, but is missing a check
> for the size of the V1 server-list payload header after determining that's
> what we've been given.
> 
> Fix this by getting rid of the the pointer to the basic header and just
> assuming that we have a V1 server-list payload and moving the V1 server
> list pointer inside the if-statement.  Dealing with other types and
> versions can be left for when such have been defined.
> 
> This can be tested by doing the following with KASAN enabled:
> 
>         echo -n -e '\x0\x0\x1\x2' | keyctl padd dns_resolver foo @p
> 
> and produces an oops like the following:
> 
>         BUG: KASAN: slab-out-of-bounds in dns_resolver_preparse+0xc9f/0xd60 net/dns_resolver/dns_key.c:127
>         Read of size 1 at addr ffff888028894084 by task syz-executor265/5069
>         ...
>         Call Trace:
>          <TASK>
>          __dump_stack lib/dump_stack.c:88 [inline]
>          dump_stack_lvl+0xd9/0x1b0 lib/dump_stack.c:106
>          print_address_description mm/kasan/report.c:377 [inline]
>          print_report+0xc3/0x620 mm/kasan/report.c:488
>          kasan_report+0xd9/0x110 mm/kasan/report.c:601
>          dns_resolver_preparse+0xc9f/0xd60 net/dns_resolver/dns_key.c:127
>          __key_create_or_update+0x453/0xdf0 security/keys/key.c:842
>          key_create_or_update+0x42/0x50 security/keys/key.c:1007
>          __do_sys_add_key+0x29c/0x450 security/keys/keyctl.c:134
>          do_syscall_x64 arch/x86/entry/common.c:52 [inline]
>          do_syscall_64+0x40/0x110 arch/x86/entry/common.c:83
>          entry_SYSCALL_64_after_hwframe+0x62/0x6a
> 
> This patch was originally by Edward Adam Davis, but was modified by Linus.
> 
> Fixes: b946001d3bb1 ("keys, dns: Allow key types (eg. DNS) to be reclaimed immediately on expiry")
> Reported-and-tested-by: syzbot+94bbb75204a05da3d89f@syzkaller.appspotmail.com
> Link: https://lore.kernel.org/r/0000000000009b39bc060c73e209@google.com/
> Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
> Signed-off-by: Edward Adam Davis <eadavis@qq.com>
> Signed-off-by: David Howells <dhowells@redhat.com>
> Tested-by: David Howells <dhowells@redhat.com>
> cc: Edward Adam Davis <eadavis@qq.com>
> cc: Simon Horman <horms@kernel.org>
> cc: Linus Torvalds <torvalds@linux-foundation.org>
> cc: Jarkko Sakkinen <jarkko@kernel.org>
> cc: Jeffrey E Altman <jaltman@auristor.com>
> cc: Wang Lei <wang840925@gmail.com>
> cc: Jeff Layton <jlayton@redhat.com>
> cc: Steve French <sfrench@us.ibm.com>
> cc: Marc Dionne <marc.dionne@auristor.com>
> cc: "David S. Miller" <davem@davemloft.net>
> cc: Eric Dumazet <edumazet@google.com>
> cc: Jakub Kicinski <kuba@kernel.org>
> cc: Paolo Abeni <pabeni@redhat.com>
> cc: linux-afs@lists.infradead.org
> cc: linux-cifs@vger.kernel.org
> cc: linux-nfs@vger.kernel.org
> cc: ceph-devel@vger.kernel.org
> cc: keyrings@vger.kernel.org
> cc: netdev@vger.kernel.org
> ---
>  net/dns_resolver/dns_key.c |   19 +++++++++----------
>  1 file changed, 9 insertions(+), 10 deletions(-)
> 
> diff --git a/net/dns_resolver/dns_key.c b/net/dns_resolver/dns_key.c
> index 2a6d363763a2..f18ca02aa95a 100644
> --- a/net/dns_resolver/dns_key.c
> +++ b/net/dns_resolver/dns_key.c
> @@ -91,8 +91,6 @@ const struct cred *dns_resolver_cache;
>  static int
>  dns_resolver_preparse(struct key_preparsed_payload *prep)
>  {
> -	const struct dns_server_list_v1_header *v1;
> -	const struct dns_payload_header *bin;
>  	struct user_key_payload *upayload;
>  	unsigned long derrno;
>  	int ret;
> @@ -103,27 +101,28 @@ dns_resolver_preparse(struct key_preparsed_payload *prep)
>  		return -EINVAL;
>  
>  	if (data[0] == 0) {
> +		const struct dns_server_list_v1_header *v1;
> +
>  		/* It may be a server list. */
> -		if (datalen <= sizeof(*bin))
> +		if (datalen <= sizeof(*v1))
>  			return -EINVAL;
>  
> -		bin = (const struct dns_payload_header *)data;
> -		kenter("[%u,%u],%u", bin->content, bin->version, datalen);
> -		if (bin->content != DNS_PAYLOAD_IS_SERVER_LIST) {
> +		v1 = (const struct dns_server_list_v1_header *)data;
> +		kenter("[%u,%u],%u", v1->hdr.content, v1->hdr.version, datalen);
> +		if (v1->hdr.content != DNS_PAYLOAD_IS_SERVER_LIST) {
>  			pr_warn_ratelimited(
>  				"dns_resolver: Unsupported content type (%u)\n",
> -				bin->content);
> +				v1->hdr.content);
>  			return -EINVAL;
>  		}
>  
> -		if (bin->version != 1) {
> +		if (v1->hdr.version != 1) {
>  			pr_warn_ratelimited(
>  				"dns_resolver: Unsupported server list version (%u)\n",
> -				bin->version);
> +				v1->hdr.version);
>  			return -EINVAL;
>  		}
>  
> -		v1 = (const struct dns_server_list_v1_header *)bin;
>  		if ((v1->status != DNS_LOOKUP_GOOD &&
>  		     v1->status != DNS_LOOKUP_GOOD_WITH_BAD)) {
>  			if (prep->expiry == TIME64_MAX)
> 

Hi Edward and kernel experts,

  Above patch(upstream commit: 1997b3cb4217b09) seems causing a keyctl05 case
to fail in LTP:
https://github.com/linux-test-project/ltp/blob/master/testcases/kernel/syscalls/keyctl/keyctl05.c

It could be reproduced on a bare metal platform.
Kconfig: https://raw.githubusercontent.com/xupengfe/kconfig_diff/main/config_v6.7-rc8
Seems general kconfig could reproduce this issue.

  Bisected info between v6.7-rc7(keyctl05 passed) and v6.7-rc8(keyctl05 failed)
is in attached.

keyctl05 failed in add_key with type "dns_resolver" syscall step tracked
by strace:
"
[pid 863107] add_key("dns_resolver", "desc", "\0\0\1\377\0", 5, KEY_SPEC_SESSION_KEYRING <unfinished ...>
[pid 863106] <... alarm resumed>)       = 30
[pid 863107] <... add_key resumed>)     = -1 EINVAL (Invalid argument)
"

Passed behavior in v6.7-rc7 kernel:
"
[pid  6726] add_key("dns_resolver", "desc", "\0\0\1\377\0", 5, KEY_SPEC_SESSION_KEYRING <unfinished ...>
[pid  6725] rt_sigreturn({mask=[]})     = 61
[pid  6726] <... add_key resumed>)      = 1029222644
"

Do you mind to take a look for above issue?

Best Regards,
Thanks!

--CKlVRO8OFc+GHLsw
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: attachment; filename="bisect.log"

git bisect start
# status: waiting for both good and bad commits
# good: [861deac3b092f37b2c5e6871732f3e11486f7082] Linux 6.7-rc7
git bisect good 861deac3b092f37b2c5e6871732f3e11486f7082
# status: waiting for bad commit, 1 good commit known
# bad: [610a9b8f49fbcf1100716370d3b5f6f884a2835a] Linux 6.7-rc8
git bisect bad 610a9b8f49fbcf1100716370d3b5f6f884a2835a
# good: [861deac3b092f37b2c5e6871732f3e11486f7082] Linux 6.7-rc7
git bisect good 861deac3b092f37b2c5e6871732f3e11486f7082
# bad: [505e701c0b2cfa9e34811020829759b7663a604c] Merge tag 'kbuild-fixes-v6.7-2' of git://git.kernel.org/pub/scm/linux/kernel/git/masahiroy/linux-kbuild
git bisect bad 505e701c0b2cfa9e34811020829759b7663a604c
# good: [1803d0c5ee1a3bbee23db2336e21add067824f02] mailmap: add an old address for Naoya Horiguchi
git bisect good 1803d0c5ee1a3bbee23db2336e21add067824f02
# bad: [eeec2599630ac1ac03db98f3ba976975c72a1427] Merge tag 'bcachefs-2023-12-27' of https://evilpiepirate.org/git/bcachefs
git bisect bad eeec2599630ac1ac03db98f3ba976975c72a1427
# bad: [f5837722ffecbbedf1b1dbab072a063565f0dad1] Merge tag 'mm-hotfixes-stable-2023-12-27-15-00' of git://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm
git bisect bad f5837722ffecbbedf1b1dbab072a063565f0dad1
# good: [b8e0792449928943c15d1af9f63816911d139267] virtio_blk: fix snprintf truncation compiler warning
git bisect good b8e0792449928943c15d1af9f63816911d139267
# bad: [1997b3cb4217b09e49659b634c94da47f0340409] keys, dns: Fix missing size check of V1 server-list header
git bisect bad 1997b3cb4217b09e49659b634c94da47f0340409
# good: [fbafc3e621c3f4ded43720fdb1d6ce1728ec664e] Merge tag 'for_linus' of git://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost
git bisect good fbafc3e621c3f4ded43720fdb1d6ce1728ec664e
# first bad commit: [1997b3cb4217b09e49659b634c94da47f0340409] keys, dns: Fix missing size check of V1 server-list header

--CKlVRO8OFc+GHLsw--