[PATCH 0/2] isofs: tie the name conversion buffer to its bound

[PATCH 0/2] isofs: tie the name conversion buffer to its bound

[Matthias Goergens]

Two cleanups around the scratch buffer isofs_readdir() and
isofs_lookup() hand to the name converters.  No functional change; the
first patch's assertions hold on the current code.

Found while looking at something else: get_joliet_filename() has been
passing PAGE_SIZE as its output bound since b2eb2e288604 shrank the
buffer to 1024, which is safe only because de->name_len is a single
byte.  That seemed worth writing down in a form the compiler checks.

Matthias Goergens (2):
  isofs: size the name conversion buffer from a named constant
  isofs: bound the Joliet iocharset conversion by the output buffer

 fs/isofs/dir.c    |  9 ++++++++-
 fs/isofs/isofs.h  |  9 +++++++++
 fs/isofs/joliet.c | 25 ++++++++++++++++++++-----
 fs/isofs/namei.c  |  2 +-
 fs/isofs/rock.c   |  3 +++
 5 files changed, 41 insertions(+), 7 deletions(-)


base-commit: 40288c9206c17eb66a603262e06a58d300d0f279
-- 
2.55.0

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

[Matthias Goergens]

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@gmail.com>
---
 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@infradead.org> 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

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@gmail.com>

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@infradead.org> 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@suse.com>
SUSE Labs, CR

[PATCH 2/2] isofs: bound the Joliet iocharset conversion by the output buffer

[Matthias Goergens]

uni16_to_x8() has no output-size parameter.  It passes
NLS_MAX_CHARSET_SIZE to uni2char() on every iteration, which bounds what
that one character may write but says nothing about how much of the
buffer is left, and then writes a terminator wherever it stopped.

Nothing bounds the total.  It fits today only because the caller's input
is bounded: de->name_len is a single byte, so at most 255 >> 1 units,
each expanding to at most NLS_MAX_CHARSET_SIZE bytes.  The previous
patch asserts that relationship at build time, but the conversion itself
should not depend on it.

Pass the buffer size in and hand uni2char() the space that actually
remains, stopping on -ENAMETOOLONG, as fs/hfsplus/unicode.c does.

The result is unchanged for every name a valid image can carry; only a
caller that passed a smaller buffer would now truncate rather than
overrun.  Listings of plain, Rock Ridge, Joliet, Rock-Ridge-plus-Joliet
and zisofs images are unchanged.

Signed-off-by: Matthias Goergens <matthias.goergens@gmail.com>
---
 fs/isofs/joliet.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/fs/isofs/joliet.c b/fs/isofs/joliet.c
index b1f4a105ee87..0832f40a9a2b 100644
--- a/fs/isofs/joliet.c
+++ b/fs/isofs/joliet.c
@@ -15,19 +15,26 @@
  * Convert Unicode 16 to UTF-8 or ASCII.
  */
 static int
-uni16_to_x8(unsigned char *ascii, __be16 *uni, int len, struct nls_table *nls)
+uni16_to_x8(unsigned char *ascii, __be16 *uni, int len, struct nls_table *nls,
+	    int outsize)
 {
 	__be16 *ip, ch;
-	unsigned char *op;
+	unsigned char *op, *end;
 
 	ip = uni;
 	op = ascii;
+	end = ascii + outsize - 1;	/* leave room for the terminator */
 
 	while ((ch = get_unaligned(ip)) && len) {
 		int llen;
-		llen = nls->uni2char(be16_to_cpu(ch), op, NLS_MAX_CHARSET_SIZE);
+
+		if (op >= end)
+			break;
+		llen = nls->uni2char(be16_to_cpu(ch), op, end - op);
 		if (llen > 0)
 			op += llen;
+		else if (llen == -ENAMETOOLONG)
+			break;
 		else
 			*op++ = '?';
 		ip++;
@@ -59,7 +66,8 @@ get_joliet_filename(struct iso_directory_record * de, unsigned char *outname, st
 				outname, ISOFS_NAME_BUF_SIZE);
 	} else {
 		len = uni16_to_x8(outname, (__be16 *) de->name,
-				de->name_len[0] >> 1, nls);
+				de->name_len[0] >> 1, nls,
+				ISOFS_NAME_BUF_SIZE);
 	}
 	if ((len > 2) && (outname[len-2] == ';') && (outname[len-1] == '1'))
 		len -= 2;
-- 
2.55.0

Re: [PATCH 2/2] isofs: bound the Joliet iocharset conversion by the output buffer

[Jan Kara]

On Tue 22-09-26 23:55:24, Matthias Goergens wrote:
> uni16_to_x8() has no output-size parameter.  It passes
> NLS_MAX_CHARSET_SIZE to uni2char() on every iteration, which bounds what
> that one character may write but says nothing about how much of the
> buffer is left, and then writes a terminator wherever it stopped.
> 
> Nothing bounds the total.  It fits today only because the caller's input
> is bounded: de->name_len is a single byte, so at most 255 >> 1 units,
> each expanding to at most NLS_MAX_CHARSET_SIZE bytes.  The previous
> patch asserts that relationship at build time, but the conversion itself
> should not depend on it.
> 
> Pass the buffer size in and hand uni2char() the space that actually
> remains, stopping on -ENAMETOOLONG, as fs/hfsplus/unicode.c does.
> 
> The result is unchanged for every name a valid image can carry; only a
> caller that passed a smaller buffer would now truncate rather than
> overrun.  Listings of plain, Rock Ridge, Joliet, Rock-Ridge-plus-Joliet
> and zisofs images are unchanged.
> 
> Signed-off-by: Matthias Goergens <matthias.goergens@gmail.com>

This looks mostly good except that I'd prefer get_joliet_filename() gets
the buffer length as an argument and just passes it down instead of
hardcoding it. That way the place allocating the buffer can just pass
proper size.

								Honza

> ---
>  fs/isofs/joliet.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/isofs/joliet.c b/fs/isofs/joliet.c
> index b1f4a105ee87..0832f40a9a2b 100644
> --- a/fs/isofs/joliet.c
> +++ b/fs/isofs/joliet.c
> @@ -15,19 +15,26 @@
>   * Convert Unicode 16 to UTF-8 or ASCII.
>   */
>  static int
> -uni16_to_x8(unsigned char *ascii, __be16 *uni, int len, struct nls_table *nls)
> +uni16_to_x8(unsigned char *ascii, __be16 *uni, int len, struct nls_table *nls,
> +	    int outsize)
>  {
>  	__be16 *ip, ch;
> -	unsigned char *op;
> +	unsigned char *op, *end;
>  
>  	ip = uni;
>  	op = ascii;
> +	end = ascii + outsize - 1;	/* leave room for the terminator */
>  
>  	while ((ch = get_unaligned(ip)) && len) {
>  		int llen;
> -		llen = nls->uni2char(be16_to_cpu(ch), op, NLS_MAX_CHARSET_SIZE);
> +
> +		if (op >= end)
> +			break;
> +		llen = nls->uni2char(be16_to_cpu(ch), op, end - op);
>  		if (llen > 0)
>  			op += llen;
> +		else if (llen == -ENAMETOOLONG)
> +			break;
>  		else
>  			*op++ = '?';
>  		ip++;
> @@ -59,7 +66,8 @@ get_joliet_filename(struct iso_directory_record * de, unsigned char *outname, st
>  				outname, ISOFS_NAME_BUF_SIZE);
>  	} else {
>  		len = uni16_to_x8(outname, (__be16 *) de->name,
> -				de->name_len[0] >> 1, nls);
> +				de->name_len[0] >> 1, nls,
> +				ISOFS_NAME_BUF_SIZE);
>  	}
>  	if ((len > 2) && (outname[len-2] == ';') && (outname[len-1] == '1'))
>  		len -= 2;
> -- 
> 2.55.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 0/2] isofs: tie the name conversion buffer to its bound

[Jan Kara]

On Tue 22-09-26 23:55:22, Matthias Goergens wrote:
> Two cleanups around the scratch buffer isofs_readdir() and
> isofs_lookup() hand to the name converters.  No functional change; the
> first patch's assertions hold on the current code.
> 
> Found while looking at something else: get_joliet_filename() has been
> passing PAGE_SIZE as its output bound since b2eb2e288604 shrank the
> buffer to 1024, which is safe only because de->name_len is a single
> byte.  That seemed worth writing down in a form the compiler checks.

Well, the buffer indeed was allocated PAGE_SIZE large but the space beyond
offset 1024 was used for the storage of iso_directory_record loaded from
the disk (tmpde). So the storage available for the name was only 1024 bytes
large from the start. Only the failures would be more subtle if the
overflow ever happened before b2eb2e288604. That being said I agree that
get_joliet_filename() usage is outright wrong (despite being harmless) and
making buffer size handling logic more robust is useful.

								Honza

> 
> Matthias Goergens (2):
>   isofs: size the name conversion buffer from a named constant
>   isofs: bound the Joliet iocharset conversion by the output buffer
> 
>  fs/isofs/dir.c    |  9 ++++++++-
>  fs/isofs/isofs.h  |  9 +++++++++
>  fs/isofs/joliet.c | 25 ++++++++++++++++++++-----
>  fs/isofs/namei.c  |  2 +-
>  fs/isofs/rock.c   |  3 +++
>  5 files changed, 41 insertions(+), 7 deletions(-)
> 
> 
> base-commit: 40288c9206c17eb66a603262e06a58d300d0f279
> -- 
> 2.55.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR