| Message ID | 1843374.1703172614@warthog.procyon.org.uk |
|---|---|
| State | New |
| Headers | show |
| Series | [GIT,PULL] afs, dns: Fix dynamic root interaction with negative DNS | expand |
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!
+ 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;
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
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> ...
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(-)