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;
