Linux-Fsdevel Archive on lore.kernel.org
help / color / mirror / Atom feed
* [RFC][PATCH] afs: Work around strnlen() oops with CONFIG_FORTIFIED_SOURCE=y
@ 2020-12-21 16:14 David Howells
  2020-12-21 17:37 ` Linus Torvalds
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: David Howells @ 2020-12-21 16:14 UTC (permalink / raw)
  To: Daniel Axtens
  Cc: dhowells, torvalds, marc.dionne, linux-afs, linux-fsdevel, linux-kernel

[cc'ing mailing lists]

Hi Daniel,

CONFIG_FORTIFIED_SOURCE=y now causes an oops in strnlen() from afs (see
attached patch for an explanation).  Is replacing the use with memchr() the
right approach?  Or should I be calling __real_strnlen() or whatever it's
called?

David
---
From: David Howells <dhowells@redhat.com>

afs: Work around strnlen() oops with CONFIG_FORTIFIED_SOURCE=y

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>
cc: Daniel Axtens <dja@axtens.net>

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;


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [RFC][PATCH] afs: Work around strnlen() oops with CONFIG_FORTIFIED_SOURCE=y
  2020-12-21 16:14 [RFC][PATCH] afs: Work around strnlen() oops with CONFIG_FORTIFIED_SOURCE=y David Howells
@ 2020-12-21 17:37 ` Linus Torvalds
  2021-01-01  1:18 ` Daniel Axtens
  2021-01-04 12:32 ` David Howells
  2 siblings, 0 replies; 6+ messages in thread
From: Linus Torvalds @ 2020-12-21 17:37 UTC (permalink / raw)
  To: David Howells
  Cc: Daniel Axtens, Marc Dionne, linux-afs, linux-fsdevel,
	Linux Kernel Mailing List

On Mon, Dec 21, 2020 at 8:14 AM David Howells <dhowells@redhat.com> wrote:
>
> CONFIG_FORTIFIED_SOURCE=y now causes an oops in strnlen() from afs (see
> attached patch for an explanation).  Is replacing the use with memchr() the
> right approach?  Or should I be calling __real_strnlen() or whatever it's
> called?

Ugh. No.

> 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.

I htink the right fix would be to try to create a type that actually
describes that.

IOW, maybe the afs_xdr_dirent union could be written something like

  union afs_xdr_dirent {
          struct {
                  u8              valid;
                  u8              unused[1];
                  __be16          hash_next;
                  __be32          vnode;
                  __be32          unique;
                  u8              name[];
         } u;
          u8                      extended_name[32];
  } __packed;

instead, and have a big comment about how "name[]" is that
"16+overflow+next entries" thing?

I didn't check how you currently use that ->name thing (not a good
identifier to grep for..), so you might want some other model - like
using a separate union case for this "unconstrained name" case.

In fact, maybe that separate union struct is a better model anyway, to
act as even more of documentation about the different cases..

              Linus

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [RFC][PATCH] afs: Work around strnlen() oops with CONFIG_FORTIFIED_SOURCE=y
  2020-12-21 16:14 [RFC][PATCH] afs: Work around strnlen() oops with CONFIG_FORTIFIED_SOURCE=y David Howells
  2020-12-21 17:37 ` Linus Torvalds
@ 2021-01-01  1:18 ` Daniel Axtens
  2021-01-04 12:32 ` David Howells
  2 siblings, 0 replies; 6+ messages in thread
From: Daniel Axtens @ 2021-01-01  1:18 UTC (permalink / raw)
  To: David Howells
  Cc: dhowells, torvalds, marc.dionne, linux-afs, linux-fsdevel, linux-kernel

Hi David,

> CONFIG_FORTIFIED_SOURCE=y now causes an oops in strnlen() from afs (see
> attached patch for an explanation).  Is replacing the use with memchr() the
> right approach?  Or should I be calling __real_strnlen() or whatever it's
> called?

You certainly shouldn't be calling __real_strnlen().

memchr() is probably the right answer if you want a small fix.

However, as Linus suggested, the 'right' answer is to re-engineer your
data structures so that the string operations you do don't overflow
their arrays.

Kind regards,
Daniel

>
> David
> ---
> From: David Howells <dhowells@redhat.com>
>
> afs: Work around strnlen() oops with CONFIG_FORTIFIED_SOURCE=y
>
> 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>
> cc: Daniel Axtens <dja@axtens.net>
>
> 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;

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [RFC][PATCH] afs: Work around strnlen() oops with CONFIG_FORTIFIED_SOURCE=y
  2020-12-21 16:14 [RFC][PATCH] afs: Work around strnlen() oops with CONFIG_FORTIFIED_SOURCE=y David Howells
  2020-12-21 17:37 ` Linus Torvalds
  2021-01-01  1:18 ` Daniel Axtens
@ 2021-01-04 12:32 ` David Howells
  2021-01-04 18:17   ` Linus Torvalds
  2021-01-04 21:09   ` David Howells
  2 siblings, 2 replies; 6+ messages in thread
From: David Howells @ 2021-01-04 12:32 UTC (permalink / raw)
  To: Linus Torvalds, Daniel Axtens
  Cc: dhowells, Marc Dionne, linux-afs, linux-fsdevel,
	Linux Kernel Mailing List

Linus Torvalds <torvalds@linux-foundation.org> wrote:

> I htink the right fix would be to try to create a type that actually
> describes that.

How about the attached, then?  It's basically what you suggested.

I realised whilst doing this that I'm not quite getting the extended-name
handling correct, but I'll deal with that in a separate patch.

David
---
commit 26982a89cad77c0efc1c0c79bee0e3d75e9281d4
Author: David Howells <dhowells@redhat.com>
Date:   Mon Dec 21 22:37:58 2020 +0000

    afs: Work around strnlen() oops with CONFIG_FORTIFIED_SOURCE=y
    
    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 changing the afs_xdr_dirent union name field to an
    indeterminate-length array and dropping the overflow field.
    
    (Note that whilst looking at this, I realised that the calculation of the
    number of slots a dirent used is non-standard and not quite right, but I'll
    address that in a separate patch.)
    
    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>

diff --git a/fs/afs/xdr_fs.h b/fs/afs/xdr_fs.h
index 94f1f398eefa..c926430fd08a 100644
--- a/fs/afs/xdr_fs.h
+++ b/fs/afs/xdr_fs.h
@@ -54,10 +54,15 @@ union afs_xdr_dirent {
 		__be16		hash_next;
 		__be32		vnode;
 		__be32		unique;
-		u8		name[16];
-		u8		overflow[4];	/* if any char of the name (inc
-						 * NUL) reaches here, consume
-						 * the next dirent too */
+		u8		name[];
+		/* When determining the number of dirent slots needed to
+		 * represent a directory entry, name should be assumed to be 16
+		 * bytes, due to a now-standardised (mis)calculation, but it is
+		 * in fact 20 bytes in size.
+		 *
+		 * For names longer than (16 or) 20 bytes, extra slots should
+		 * be annexed to this one using the extended_name format.
+		 */
 	} u;
 	u8			extended_name[32];
 } __packed;


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [RFC][PATCH] afs: Work around strnlen() oops with CONFIG_FORTIFIED_SOURCE=y
  2021-01-04 12:32 ` David Howells
@ 2021-01-04 18:17   ` Linus Torvalds
  2021-01-04 21:09   ` David Howells
  1 sibling, 0 replies; 6+ messages in thread
From: Linus Torvalds @ 2021-01-04 18:17 UTC (permalink / raw)
  To: David Howells
  Cc: Daniel Axtens, Marc Dionne, linux-afs, linux-fsdevel,
	Linux Kernel Mailing List

On Mon, Jan 4, 2021 at 4:32 AM David Howells <dhowells@redhat.com> wrote:
>
> How about the attached, then?  I

That looks like the right thing, but I didn't check how the name[]
array (or the overflow[] one) is actually used. But I assume you've
tested this.

Do you want me to apply the patch as-is, or will I be getting a pull
request with this (and the number-of-slots calculation thing you
mention in the commit message)?

               Linus

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [RFC][PATCH] afs: Work around strnlen() oops with CONFIG_FORTIFIED_SOURCE=y
  2021-01-04 12:32 ` David Howells
  2021-01-04 18:17   ` Linus Torvalds
@ 2021-01-04 21:09   ` David Howells
  1 sibling, 0 replies; 6+ messages in thread
From: David Howells @ 2021-01-04 21:09 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: dhowells, Daniel Axtens, Marc Dionne, linux-afs, linux-fsdevel,
	Linux Kernel Mailing List

Linus Torvalds <torvalds@linux-foundation.org> wrote:

> array (or the overflow[] one) is actually used. But I assume you've
> tested this.
> 
> Do you want me to apply the patch as-is, or will I be getting a pull
> request with this (and the number-of-slots calculation thing you
> mention in the commit message)?

I can give you a pull req for them as a pair.  I don't know if Daniel wants to
comment on the first patch.

David


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2021-01-04 21:11 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-21 16:14 [RFC][PATCH] afs: Work around strnlen() oops with CONFIG_FORTIFIED_SOURCE=y David Howells
2020-12-21 17:37 ` Linus Torvalds
2021-01-01  1:18 ` Daniel Axtens
2021-01-04 12:32 ` David Howells
2021-01-04 18:17   ` Linus Torvalds
2021-01-04 21:09   ` David Howells

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).