All of lore.kernel.org
 help / color / mirror / Atom feed
From: David Howells <dhowells@redhat.com>
To: linux-afs@lists.infradead.org
Cc: dhowells@redhat.com, marc.dionne@auristor.com,
	linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: [PATCH] afs: Work around strnlen() oops with CONFIG_FORTIFIED_SOURCE=y
Date: Wed, 23 Dec 2020 13:49:10 +0000	[thread overview]
Message-ID: <160873135094.834130.9048269997292829364.stgit@warthog.procyon.org.uk> (raw)

AFS has a structured layout in its directory contents (AFS dirs are
downloaded as files and parsed locally by the client for lookup/readdir).
The slots in the directory are defined by union afs_xdr_dirent.  This,
however, only directly allows a name of a length that will fit into that
union.  To support a longer name, the next 1-8 contiguous entries are
annexed to the first one and the name flows across these.

afs_dir_iterate_block() uses strnlen(), limited to the space to the end of
the page, to find out how long the name is.  This worked fine until
6a39e62abbaf.  With that commit, the compiler determines the size of the
array and asserts that the string fits inside that array.  This is a
problem for AFS because we *expect* it to overflow one or more arrays.

A similar problem also occurs in afs_dir_scan_block() when a directory file
is being locally edited to avoid the need to redownload it.  There strlen()
was being used safely because each page has the last byte set to 0 when the
file is downloaded and validated (in afs_dir_check_page()).

Fix this by using memchr() instead and hoping no one changes that to check
the object size.

The issue can be triggered by something like:

	touch /afs/example.com/thisisaveryveryverylongname

and it generates a report that looks like:

	detected buffer overflow in strnlen
	------------[ cut here ]------------
	kernel BUG at lib/string.c:1149!
	...
	RIP: 0010:fortify_panic+0xf/0x11
	...
	Call Trace:
	 afs_dir_iterate_block+0x12b/0x35b
	 afs_dir_iterate+0x14e/0x1ce
	 afs_do_lookup+0x131/0x417
	 afs_lookup+0x24f/0x344
	 lookup_open.isra.0+0x1bb/0x27d
	 open_last_lookups+0x166/0x237
	 path_openat+0xe0/0x159
	 do_filp_open+0x48/0xa4
	 ? kmem_cache_alloc+0xf5/0x16e
	 ? __clear_close_on_exec+0x13/0x22
	 ? _raw_spin_unlock+0xa/0xb
	 do_sys_openat2+0x72/0xde
	 do_sys_open+0x3b/0x58
	 do_syscall_64+0x2d/0x3a
	 entry_SYSCALL_64_after_hwframe+0x44/0xa9

Fixes: 6a39e62abbaf ("lib: string.h: detect intra-object overflow in fortified string functions")
Reported-by: Marc Dionne <marc.dionne@auristor.com>
Signed-off-by: David Howells <dhowells@redhat.com>
Tested-by: Marc Dionne <marc.dionne@auristor.com>
cc: Daniel Axtens <dja@axtens.net>
---

 fs/afs/dir.c      |   13 ++++++++++---
 fs/afs/dir_edit.c |    8 +++++++-
 2 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/fs/afs/dir.c b/fs/afs/dir.c
index 9068d5578a26..4fafb4e4d0df 100644
--- a/fs/afs/dir.c
+++ b/fs/afs/dir.c
@@ -350,6 +350,7 @@ static int afs_dir_iterate_block(struct afs_vnode *dvnode,
 				 unsigned blkoff)
 {
 	union afs_xdr_dirent *dire;
+	const u8 *p;
 	unsigned offset, next, curr;
 	size_t nlen;
 	int tmp;
@@ -378,9 +379,15 @@ static int afs_dir_iterate_block(struct afs_vnode *dvnode,
 
 		/* got a valid entry */
 		dire = &block->dirents[offset];
-		nlen = strnlen(dire->u.name,
-			       sizeof(*block) -
-			       offset * sizeof(union afs_xdr_dirent));
+		p = memchr(dire->u.name, 0,
+			   sizeof(*block) - offset * sizeof(union afs_xdr_dirent));
+		if (!p) {
+			_debug("ENT[%zu.%u]: %u unterminated dirent name",
+			       blkoff / sizeof(union afs_xdr_dir_block),
+			       offset, next);
+			return afs_bad(dvnode, afs_file_error_dir_over_end);
+		}
+		nlen = p - dire->u.name;
 
 		_debug("ENT[%zu.%u]: %s %zu \"%s\"",
 		       blkoff / sizeof(union afs_xdr_dir_block), offset,
diff --git a/fs/afs/dir_edit.c b/fs/afs/dir_edit.c
index 2ffe09abae7f..5ee4e992ed8f 100644
--- a/fs/afs/dir_edit.c
+++ b/fs/afs/dir_edit.c
@@ -111,6 +111,8 @@ static int afs_dir_scan_block(union afs_xdr_dir_block *block, struct qstr *name,
 			      unsigned int blocknum)
 {
 	union afs_xdr_dirent *de;
+	const u8 *p;
+	unsigned long offset;
 	u64 bitmap;
 	int d, len, n;
 
@@ -135,7 +137,11 @@ static int afs_dir_scan_block(union afs_xdr_dir_block *block, struct qstr *name,
 			continue;
 
 		/* The block was NUL-terminated by afs_dir_check_page(). */
-		len = strlen(de->u.name);
+		offset = (unsigned long)de->u.name & (PAGE_SIZE - 1);
+		p = memchr(de->u.name, 0, PAGE_SIZE - offset);
+		if (!p)
+			return -1;
+		len = p - de->u.name;
 		if (len == name->len &&
 		    memcmp(de->u.name, name->name, name->len) == 0)
 			return d;



             reply	other threads:[~2020-12-23 13:51 UTC|newest]

Thread overview: 2+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-12-23 13:49 David Howells [this message]
2020-12-23 13:53 ` [PATCH] afs: Work around strnlen() oops with CONFIG_FORTIFIED_SOURCE=y David Howells

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=160873135094.834130.9048269997292829364.stgit@warthog.procyon.org.uk \
    --to=dhowells@redhat.com \
    --cc=linux-afs@lists.infradead.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=marc.dionne@auristor.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.