[bug report] afs: Locally edit directory data for mkdir/create/unlink/...

[David Howells]

Dan Carpenter <dan.carpenter at oracle.com> wrote:

> The patch 63a4681ff39c: "afs: Locally edit directory data for
> mkdir/create/unlink/..." from Apr 6, 2018, leads to the following
> static checker warning:
> 
> 	fs/afs/dir.c:1392 afs_create()
> 	warn: 'dentry->d_name.len' is out of bounds '255' vs '15'

In this case, the static checker doesn't understand the situation
sufficiently.

>    331          memcpy(de->u.name, name->name, name->len + 1);
>                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>    332          de->u.name[name->len] = 0;
>                 ^^^^^^^^^^^^^^^^^^^^^^^^
> de->u.name is a 16 character buffer.

By the strict definition of the C type:

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

this it true - however, it's more complicated than that.

This struct represents an individual element within the structured directory
content.  These records are fixed size and you contiguously allocate however
many of them you need to store the entire record - up to 9 for a 255-char
name.

There's an overflow buffer into which the name is allowed to overflow - as far
as I can tell it used to be that if the name transgressed into this, you had
to allocate an extension record anyway, whether or not you used it.

Also, you'll notice that the record is a union and that the other member of
the union is just an array containing an extension segment for the name.

>   1364          if (dentry->d_name.len >= AFSNAMEMAX)
>                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> Here we ensure that d_name.len is 255 or less.

That's correct.

>   1392                  afs_edit_dir_add(dvnode, &dentry->d_name, &newfid,
>                                                   ^^^^^^^^^^^^^^
> This is where the warning gets generated because 255 is more than 15.

afs_dir_edit_add() allocates however many slots it needs within a page:

	/* Work out how many slots we're going to need. */
	need_slots = round_up(12 + name->len + 1 + 4, AFS_DIR_DIRENT_SIZE);
	need_slots /= AFS_DIR_DIRENT_SIZE;

David