Re: [RFC][PATCH] afs: Work around strnlen() oops with CONFIG_FORTIFIED_SOURCE=y

[Daniel Axtens]

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@xxxxxxxxxx>
>
> 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@xxxxxxxxxxxx>
> Signed-off-by: David Howells <dhowells@xxxxxxxxxx>
> cc: Daniel Axtens <dja@xxxxxxxxxx>
>
> 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;