Re: [PATCH 1/2] isofs: size the name conversion buffer from a named constant

[Jan Kara]

On Tue 22-09-26 23:55:23, Matthias Goergens wrote:
> isofs_readdir() and isofs_lookup() each allocate a 1024-byte scratch
> buffer that four converters write into: get_rock_ridge_filename(),
> get_joliet_filename(), get_acorn_filename() and isofs_name_translate().
> None of them is told how big it is.
> 
> get_joliet_filename() gets this wrong today.  It passes PAGE_SIZE as the
> output limit to utf16s_to_utf8s(), four times the buffer it is actually
> given.  That was correct until commit b2eb2e288604 ("isofs: Drop support
> of directory entries straddling blocks"), which shrank the allocation
> from a page to 1024 bytes in both callers without updating the bound.
> 
> No overflow is possible today: de->name_len is a single byte, so the
> largest name any converter can produce is 763 bytes, on the Joliet
> iocharset path ((255 >> 1) units, each expanding to at most
> NLS_MAX_CHARSET_SIZE bytes, plus a terminator).  But nothing says so,
> and joliet.c says the opposite.
> 
> Give the buffer a name, use it at both allocation sites and as the
> Joliet bound, and have each converter assert that its own worst case
> fits.  Shrinking the buffer, or NLS_MAX_CHARSET_SIZE growing, now fails
> the build instead of silently overflowing:
> 
>   fs/isofs/joliet.c:46:1: error: static assertion failed:
>     "(255 >> 1) * NLS_MAX_CHARSET_SIZE + 1 <= ISOFS_NAME_BUF_SIZE"
> 
> This follows fs/ntfs3, which couples its name buffer to the on-disk
> length field the same way (fs/ntfs3/dir.c: static_assert(NTFS_NAME_LEN *
> 4 < PATH_MAX)).
> 
> No functional change.
> 
> Signed-off-by: Matthias Goergens <matthias.goergens@xxxxxxxxx>

So when touching this, let's do this properly :). Isofs driver should never
return filename longer than NAME_MAX bytes. It's violating POSIX,
expectations of glibc and although VFS should be able to cope, something is
practically guaranteed to break somewhere down the road either in kernel or
userspace.

Isofs directory records have the maximum length of 255 characters so
filenames in directory entry can be at most 223 characters long - that's
the practical limit for isofs_name_translate(), get_acorn_filename() can
add 4 more but that's still well within the NAME_MAX limit and probably we
don't need to treat that in a special way.

get_rock_ridge_filename() has an internal bound check for NAME_MAX when
parsing 'NM' entries - that might better be limited by the buffer size
passed externally.

get_joliet_filename() - you're handling that in patch 2 so let's do the
discussion there.

After get_rock_ridge_filename() and get_joliet_filename() get the buffer
size and respect it, we can just shrink the buffer to NAME_MAX+1 bytes and
be done with it. No need for any size asserts or similar weird things.

								Honza


> ---
>  fs/isofs/dir.c    | 9 ++++++++-
>  fs/isofs/isofs.h  | 9 +++++++++
>  fs/isofs/joliet.c | 9 ++++++++-
>  fs/isofs/namei.c  | 2 +-
>  fs/isofs/rock.c   | 3 +++
>  5 files changed, 29 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/isofs/dir.c b/fs/isofs/dir.c
> index c7ca7603e97a..87c64612ef7f 100644
> --- a/fs/isofs/dir.c
> +++ b/fs/isofs/dir.c
> @@ -50,6 +50,13 @@ int isofs_name_translate(struct iso_directory_record *de, char *new, struct inod
>  }
>  
>  /* Acorn extensions written by Matthew Wilcox <willy@xxxxxxxxxxxxx> 1998 */
> +/*
> + * isofs_name_translate() copies at most de->name_len[0] bytes one for one,
> + * and get_acorn_filename() may append "," plus three hex digits and a NUL.
> + * de->name_len is a single byte.
> + */
> +static_assert(255 + 5 <= ISOFS_NAME_BUF_SIZE);
> +
>  int get_acorn_filename(struct iso_directory_record *de,
>  			    char *retname, struct inode *inode)
>  {
> @@ -237,7 +244,7 @@ static int isofs_readdir(struct file *file, struct dir_context *ctx)
>  	char *tmpname;
>  	struct inode *inode = file_inode(file);
>  
> -	tmpname = kmalloc(1024, GFP_KERNEL);
> +	tmpname = kmalloc(ISOFS_NAME_BUF_SIZE, GFP_KERNEL);
>  	if (tmpname == NULL)
>  		return -ENOMEM;
>  
> diff --git a/fs/isofs/isofs.h b/fs/isofs/isofs.h
> index dacb9cdae4fd..2af41cc23f38 100644
> --- a/fs/isofs/isofs.h
> +++ b/fs/isofs/isofs.h
> @@ -5,6 +5,15 @@
>  #include <linux/iso_fs.h>
>  #include <linux/unaligned.h>
>  
> +/*
> + * Scratch buffer for converting an on-disk name to its in-kernel form.
> + * Allocated by isofs_readdir() and isofs_lookup(), written by
> + * get_rock_ridge_filename(), get_joliet_filename(), get_acorn_filename()
> + * and isofs_name_translate().  Each of those asserts that its own worst
> + * case fits, next to the code that does the writing.
> + */
> +#define ISOFS_NAME_BUF_SIZE 1024
> +
>  enum isofs_file_format {
>  	isofs_file_normal = 0,
>  	isofs_file_sparse = 1,
> diff --git a/fs/isofs/joliet.c b/fs/isofs/joliet.c
> index c0f04a1e7f69..b1f4a105ee87 100644
> --- a/fs/isofs/joliet.c
> +++ b/fs/isofs/joliet.c
> @@ -38,6 +38,13 @@ uni16_to_x8(unsigned char *ascii, __be16 *uni, int len, struct nls_table *nls)
>  	return (op - ascii);
>  }
>  
> +/*
> + * The worst case is the iocharset path: de->name_len is a single byte, so
> + * at most 255 >> 1 UTF-16 units, each of which uni2char() may expand to
> + * NLS_MAX_CHARSET_SIZE bytes, plus the terminator.
> + */
> +static_assert((255 >> 1) * NLS_MAX_CHARSET_SIZE + 1 <= ISOFS_NAME_BUF_SIZE);
> +
>  int
>  get_joliet_filename(struct iso_directory_record * de, unsigned char *outname, struct inode * inode)
>  {
> @@ -49,7 +56,7 @@ get_joliet_filename(struct iso_directory_record * de, unsigned char *outname, st
>  	if (!nls) {
>  		len = utf16s_to_utf8s((const wchar_t *) de->name,
>  				de->name_len[0] >> 1, UTF16_BIG_ENDIAN,
> -				outname, PAGE_SIZE);
> +				outname, ISOFS_NAME_BUF_SIZE);
>  	} else {
>  		len = uni16_to_x8(outname, (__be16 *) de->name,
>  				de->name_len[0] >> 1, nls);
> diff --git a/fs/isofs/namei.c b/fs/isofs/namei.c
> index 010682f5901a..0146aef58b10 100644
> --- a/fs/isofs/namei.c
> +++ b/fs/isofs/namei.c
> @@ -153,7 +153,7 @@ struct dentry *isofs_lookup(struct inode *dir, struct dentry *dentry, unsigned i
>  	struct inode *inode;
>  	char *tmpname;
>  
> -	tmpname = kmalloc(1024, GFP_USER);
> +	tmpname = kmalloc(ISOFS_NAME_BUF_SIZE, GFP_USER);
>  	if (!tmpname)
>  		return ERR_PTR(-ENOMEM);
>  
> diff --git a/fs/isofs/rock.c b/fs/isofs/rock.c
> index 2628f31bd3a5..7d95443917ea 100644
> --- a/fs/isofs/rock.c
> +++ b/fs/isofs/rock.c
> @@ -204,6 +204,9 @@ static int rock_check_overflow(struct rock_state *rs, int sig)
>  /*
>   * return length of name field; 0: not found, -1: to be ignored
>   */
> +/* get_rock_ridge_filename() bounds the name it builds by NAME_MAX, plus a NUL. */
> +static_assert(NAME_MAX + 1 <= ISOFS_NAME_BUF_SIZE);
> +
>  int get_rock_ridge_filename(struct iso_directory_record *de,
>  			    char *retname, struct inode *inode)
>  {
> -- 
> 2.55.0
> 
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR