[GIT PULL] afs, dns: Fix dynamic root interaction with negative DNS

[GIT PULL] afs, dns: Fix dynamic root interaction with negative DNS

[David Howells]

Hi Linus,

Could you apply this, please?  It's intended to improve the interaction of
arbitrary lookups in the AFS dynamic root that hit DNS lookup failures[1]
where kafs behaves differently from openafs and causes some applications to
fail that aren't expecting that.  Further, negative DNS results aren't
getting removed and are causing failures to persist.

 (1) Always delete unused (particularly negative) dentries as soon as
     possible so that they don't prevent future lookups from retrying.

 (2) Fix the handling of new-style negative DNS lookups in ->lookup() to
     make them return ENOENT so that userspace doesn't get confused when
     stat succeeds but the following open on the looked up file then fails.

 (3) Fix key handling so that DNS lookup results are reclaimed almost as
     soon as they expire rather than sitting round either forever or for an
     additional 5 mins beyond a set expiry time returning EKEYEXPIRED.
     They persist for 1s as /bin/ls will do a second stat call if the first
     fails.

Reviewed-by: Jeffrey Altman <jaltman@auristor.com>

Thanks,
David

Link: https://bugzilla.kernel.org/show_bug.cgi?id=216637 [1]
Link: https://lore.kernel.org/r/20231211163412.2766147-1-dhowells@redhat.com/ # v1
Link: https://lore.kernel.org/r/20231211213233.2793525-1-dhowells@redhat.com/ # v2
Link: https://lore.kernel.org/r/20231212144611.3100234-1-dhowells@redhat.com/ # v3
Link: https://lore.kernel.org/r/20231221134558.1659214-1-dhowells@redhat.com/ # v4
---
The following changes since commit ceb6a6f023fd3e8b07761ed900352ef574010bcb:

  Linux 6.7-rc6 (2023-12-17 15:19:28 -0800)

are available in the Git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git tags/afs-fixes-20231221

for you to fetch changes up to 39299bdd2546688d92ed9db4948f6219ca1b9542:

  keys, dns: Allow key types (eg. DNS) to be reclaimed immediately on expiry (2023-12-21 13:47:38 +0000)

----------------------------------------------------------------
AFS fixes

----------------------------------------------------------------
David Howells (3):
      afs: Fix the dynamic root's d_delete to always delete unused dentries
      afs: Fix dynamic root lookup DNS check
      keys, dns: Allow key types (eg. DNS) to be reclaimed immediately on expiry

 fs/afs/dynroot.c           | 31 +++++++++++++++++--------------
 include/linux/key-type.h   |  1 +
 net/dns_resolver/dns_key.c | 10 +++++++++-
 security/keys/gc.c         | 31 +++++++++++++++++++++----------
 security/keys/internal.h   | 11 ++++++++++-
 security/keys/key.c        | 15 +++++----------
 security/keys/proc.c       |  2 +-
 7 files changed, 64 insertions(+), 37 deletions(-)

Re: [GIT PULL] afs, dns: Fix dynamic root interaction with negative DNS

[pr-tracker-bot]

The pull request you sent on Thu, 21 Dec 2023 15:30:14 +0000:

> git://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git tags/afs-fixes-20231221

has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/937fd403380023d065fd0509caa7eff639b144a0

Thank you!

-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/prtracker.html

Re: [GIT PULL] afs, dns: Fix dynamic root interaction with negative DNS

[Simon Horman]

+ Edward Adam Davis <eadavis@qq.com>

On Thu, Dec 21, 2023 at 03:30:14PM +0000, David Howells wrote:
> Hi Linus,
> 
> Could you apply this, please?  It's intended to improve the interaction of
> arbitrary lookups in the AFS dynamic root that hit DNS lookup failures[1]
> where kafs behaves differently from openafs and causes some applications to
> fail that aren't expecting that.  Further, negative DNS results aren't
> getting removed and are causing failures to persist.
> 
>  (1) Always delete unused (particularly negative) dentries as soon as
>      possible so that they don't prevent future lookups from retrying.
> 
>  (2) Fix the handling of new-style negative DNS lookups in ->lookup() to
>      make them return ENOENT so that userspace doesn't get confused when
>      stat succeeds but the following open on the looked up file then fails.
> 
>  (3) Fix key handling so that DNS lookup results are reclaimed almost as
>      soon as they expire rather than sitting round either forever or for an
>      additional 5 mins beyond a set expiry time returning EKEYEXPIRED.
>      They persist for 1s as /bin/ls will do a second stat call if the first
>      fails.
> 
> Reviewed-by: Jeffrey Altman <jaltman@auristor.com>
> 
> Thanks,
> David
> 
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=216637 [1]
> Link: https://lore.kernel.org/r/20231211163412.2766147-1-dhowells@redhat.com/ # v1
> Link: https://lore.kernel.org/r/20231211213233.2793525-1-dhowells@redhat.com/ # v2
> Link: https://lore.kernel.org/r/20231212144611.3100234-1-dhowells@redhat.com/ # v3
> Link: https://lore.kernel.org/r/20231221134558.1659214-1-dhowells@redhat.com/ # v4
> ---
> The following changes since commit ceb6a6f023fd3e8b07761ed900352ef574010bcb:
> 
>   Linux 6.7-rc6 (2023-12-17 15:19:28 -0800)
> 
> are available in the Git repository at:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git tags/afs-fixes-20231221
> 
> for you to fetch changes up to 39299bdd2546688d92ed9db4948f6219ca1b9542:
> 
>   keys, dns: Allow key types (eg. DNS) to be reclaimed immediately on expiry (2023-12-21 13:47:38 +0000)
> 
> ----------------------------------------------------------------
> AFS fixes
> 
> ----------------------------------------------------------------
> David Howells (3):
>       afs: Fix the dynamic root's d_delete to always delete unused dentries
>       afs: Fix dynamic root lookup DNS check
>       keys, dns: Allow key types (eg. DNS) to be reclaimed immediately on expiry

Hi Linus, David, Edward, Networking maintainers, all,

This is a heads up that my understanding is that the last patch introduces
a buffer overrun for which a patch has been posted. Ordinarily I would
think that the fix should go through net. But the above patches aren't in
net yet.

Given a) we're now in a holiday season and b) the severity of this
problem is unclear (to me), perhaps it is best to wait a bit then
post the fix to net?

Link: https://lore.kernel.org/netdev/tencent_7D663C8936BA96F837124A4474AF76ED6709@qq.com/

N.B. The hash in the fixes tag for the fix patch is now incorrect.

For reference the fix, from the link above, is below.
I've fixed the hash for the fixes tag and added the posted review tag.
And added my own SoB, because the patch is in this email.

From: Edward Adam Davis <eadavis@qq.com>

bin will be forcibly converted to "struct dns_server_list_v1_header *", so it 
is necessary to compare datalen with sizeof(*v1).

Fixes: 39299bdd2546 ("keys, dns: Allow key types (eg. DNS) to be reclaimed immediately on expiry")
Reported-and-tested-by: syzbot+94bbb75204a05da3d89f@syzkaller.appspotmail.com
Signed-off-by: Edward Adam Davis <eadavis@qq.com>
Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
Signed-off-by: Simon Horman <horms@kernel.org>

---
 net/dns_resolver/dns_key.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/dns_resolver/dns_key.c b/net/dns_resolver/dns_key.c
index 3233f4f25fed..15f19521021c 100644
--- a/net/dns_resolver/dns_key.c
+++ b/net/dns_resolver/dns_key.c
@@ -104,7 +104,7 @@ dns_resolver_preparse(struct key_preparsed_payload *prep)
 
 	if (data[0] == 0) {
 		/* It may be a server list. */
-		if (datalen <= sizeof(*bin))
+		if (datalen <= sizeof(*v1))
 			return -EINVAL;
 
 		bin = (const struct dns_payload_header *)data;
-- 
2.43.0

Re: [GIT PULL] afs, dns: Fix dynamic root interaction with negative DNS

[Linus Torvalds]

[-- Attachment #1: Type: text/plain, Size: 1337 bytes --]

On Sat, 23 Dec 2023 at 09:29, Simon Horman <horms@kernel.org> wrote:
>
>
>         if (data[0] == 0) {
>                 /* It may be a server list. */
> -               if (datalen <= sizeof(*bin))
> +               if (datalen <= sizeof(*v1))
>                         return -EINVAL;
>
>                 bin = (const struct dns_payload_header *)data;

Ugh, I hate how it checks the size of a *different* structure than the
one it then assigns the pointer to.

So I get the feeling that we should get rid of 'bin' entirely, and
just use the 'v1' pointer, since it literally checks that that is what
it is, and then the size check matches the thing we're casting things
to.

So then "bin->xyz" becomes "v1->hdr.xyz".

Yes, the patch becomes a bit bigger, but I think the end result is a
whole lot more obvious and maintainable.

I'd also move the remaining 'v1' variable declaration to the inner
context where it's actually used.

IOW, I personally would be much happier with a patch like the attached, but I

 (a) don't want to take credit for this, since my change is purely syntactic

 (b) have not tested this patch apart from checking that it compiles
in at least one config

so honestly, I'd love to see this patch come back to me with sign-offs
and tested-bys by the actual people who noticed this.

Hmm?

                 Linus

[-- Attachment #2: patch.diff --]
[-- Type: text/x-patch, Size: 1751 bytes --]

 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)

[PATCH] keys, dns: Fix missing size check of V1 server-list header

[David Howells]

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)

Re: [PATCH] keys, dns: Fix missing size check of V1 server-list header

[Simon Horman]

On Sun, Dec 24, 2023 at 12:02:49AM +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>

Thanks.

FWIIW, I prefer this approach where v1 and bin don't alias each other,
and the scope of v1 is constrained to the block where it is used.

Reviewed-by: Simon Horman <horms@kernel.org>

...

Re: [PATCH] keys, dns: Fix missing size check of V1 server-list header

[Pengfei Xu]

[-- Attachment #1: Type: text/plain, Size: 6595 bytes --]

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!

[-- Attachment #2: bisect.log --]
[-- Type: text/plain, Size: 1877 bytes --]

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

Re: [PATCH] keys, dns: Fix missing size check of V1 server-list header

[Edward Adam Davis]

On Wed, 10 Jan 2024 12:40:41 +0800, Pengfei Xu 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)
The reason for the failure of add_key() is that the length of the incoming data
is 5, which is less than sizeof(*v1), so keyctl05.c failed.
Suggest modifying keyctl05.c to increase the length of the incoming data to 6 
bytes or more.
> "
> 
> 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?
Edward,
BR

Re: [PATCH] keys, dns: Fix missing size check of V1 server-list header

[Pengfei Xu]

On 2024-01-10 at 12:40:41 +0800, Pengfei Xu wrote:
> 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
> "

Sorry, updated more keyctl05 failed in add_key with type "dns_resolver"
syscall step tracked by strace:
"
[pid 863107] getppid()                  = 863106
[pid 863107] kill(863106, SIGUSR1)      = 0
[pid 863106] <... wait4 resumed>0x7ffc5ec94858, 0, NULL) = ? ERESTARTSYS (To be restarted if SA_RESTART is set)
[pid 863107] keyctl(KEYCTL_JOIN_SESSION_KEYRING, NULL <unfinished ...>
[pid 863106] --- SIGUSR1 {si_signo=SIGUSR1, si_code=SI_USER, si_pid=863107, si_uid=0} ---
[pid 863107] <... keyctl resumed>)      = 512571383
[pid 863106] alarm(30 <unfinished ...>

[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)
[pid 863106] rt_sigreturn({mask=[]} <unfinished ...>
[pid 863107] write(2, "keyctl05.c:114: TFAIL: unexpecte"..., 79keyctl05.c:114: TFAIL: unexpected error adding 'dns_resolver' key: EINVAL (22)
 <unfinished ...>
[pid 863106] <... rt_sigreturn resumed>) = 61
[pid 863107] <... write resumed>)       = 79
"

Passed behavior in v6.7-rc7 kernel:
"
[pid  6726] getppid()                   = 6725
[pid  6726] kill(6725, SIGUSR1 <unfinished ...>
[pid  6725] <... wait4 resumed>0x7ffc6cad1c68, 0, NULL) = ? ERESTARTSYS (To be restarted if SA_RESTART is set)
[pid  6726] <... kill resumed>)         = 0
[pid  6725] --- SIGUSR1 {si_signo=SIGUSR1, si_code=SI_USER, si_pid=6726, si_uid=0} ---
[pid  6726] keyctl(KEYCTL_JOIN_SESSION_KEYRING, NULL <unfinished ...>
[pid  6725] alarm(30 <unfinished ...>
[pid  6726] <... keyctl resumed>)       = 713868472
[pid  6725] <... alarm resumed>)        = 30

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

> 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

Re: [PATCH] keys, dns: Fix missing size check of V1 server-list header

[Pengfei Xu]

On 2024-01-10 at 13:19:49 +0800, Edward Adam Davis wrote:
> On Wed, 10 Jan 2024 12:40:41 +0800, Pengfei Xu 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)
> The reason for the failure of add_key() is that the length of the incoming data
> is 5, which is less than sizeof(*v1), so keyctl05.c failed.
> Suggest modifying keyctl05.c to increase the length of the incoming data to 6 
> bytes or more.

Thanks for your suggestion!
dns_server_list_v1_header struct is 6 u8 data instead of previous bin.

After increased the dns_res_payload to 7 bytes(6 bytes was still failed),
keyctl05 could be passed.
"
static char dns_res_payload[] = { 0x00, 0x00, 0x01, 0xff, 0x00, 0x00, 0x00 };
"

I will improve the case in LTP.

Thanks!

> > "
> > 
> > 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?
> Edward,
> BR
> 

Re: [PATCH] keys, dns: Fix missing size check of V1 server-list header

[David Howells]

Pengfei Xu <pengfei.xu@intel.com> wrote:

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

It should fail as the payload is actually invalid.  The payload specifies a
version 1 format - and that requires a 6-byte header.  The bug the patched
fixes is that whilst there is a length check for the basic 3-byte header,
there was no length check for the extended v1 header.

> After increased the dns_res_payload to 7 bytes(6 bytes was still failed),

The following doesn't work for you?

	echo -n -e '\0\0\01\xff\0\0' | keyctl padd dns_resolver desc @p

David

Re: [PATCH] keys, dns: Fix missing size check of V1 server-list header

[Pengfei Xu]

On 2024-01-10 at 10:14:28 +0000, David Howells wrote:
> Pengfei Xu <pengfei.xu@intel.com> wrote:
> 
> >   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)
> > "
> 
> It should fail as the payload is actually invalid.  The payload specifies a
> version 1 format - and that requires a 6-byte header.  The bug the patched
> fixes is that whilst there is a length check for the basic 3-byte header,
> there was no length check for the extended v1 header.

Thanks for description!

> 
> > After increased the dns_res_payload to 7 bytes(6 bytes was still failed),
> 
> The following doesn't work for you?
> 
> 	echo -n -e '\0\0\01\xff\0\0' | keyctl padd dns_resolver desc @p

I tried as follows, 6 bytes failed and 7 bytes passed:
"
# echo -n -e '\0\0\01\xff\0\0' | keyctl padd dns_resolver desc @p
add_key: Invalid argument
# echo -n -e '\0\0\01\xff\0\0\0' | keyctl padd dns_resolver desc @p
74678921
# uname -r
6.7.0-rc8
"

Thanks!

> 
> David
> 

Re: [PATCH] keys, dns: Fix missing size check of V1 server-list header

[David Howells]

Meh.  Does the attached fix it for you?

David
---
diff --git a/net/dns_resolver/dns_key.c b/net/dns_resolver/dns_key.c
index f18ca02aa95a..c42ddd85ff1f 100644
--- a/net/dns_resolver/dns_key.c
+++ b/net/dns_resolver/dns_key.c
@@ -104,7 +104,7 @@ dns_resolver_preparse(struct key_preparsed_payload *prep)
 		const struct dns_server_list_v1_header *v1;
 
 		/* It may be a server list. */
-		if (datalen <= sizeof(*v1))
+		if (datalen < sizeof(*v1))
 			return -EINVAL;
 
 		v1 = (const struct dns_server_list_v1_header *)data;

Re: [PATCH] keys, dns: Fix missing size check of V1 server-list header

[Linus Torvalds]

On Wed, 10 Jan 2024 at 09:23, David Howells <dhowells@redhat.com> wrote:
>
> Meh.  Does the attached fix it for you?

Bah. Obvious fix is obvious.

Mind sending it as a proper patch with sign-off etc, and we'll get
this fixed and marked for stable.

           Linus