[PATCH] isofs: validate directory records in NFS get_parent

[PATCH] isofs: validate directory records in NFS get_parent

[Yichong Chen]

isofs_export_get_parent() assumes valid "." and ".." entries.

A malformed image can provide an invalid length for the first entry.

Validate both records before using the first length as the ".." offset.

This keeps the NFS export get_parent path from accepting malformed records.

Signed-off-by: Yichong Chen <chenyichong@uniontech.com>
---
 fs/isofs/export.c | 30 +++++++++++++++++++++++++++++-
 1 file changed, 29 insertions(+), 1 deletion(-)

diff --git a/fs/isofs/export.c b/fs/isofs/export.c
index 78f80c1a5c54..bffec45bb274 100644
--- a/fs/isofs/export.c
+++ b/fs/isofs/export.c
@@ -16,6 +16,26 @@
 
 #include "isofs.h"
 
+static bool isofs_dir_record_valid(struct iso_directory_record *de,
+				   unsigned long offset,
+				   unsigned long bufsize)
+{
+	unsigned int len;
+	unsigned int name_len;
+	unsigned long min_len = offsetof(struct iso_directory_record, name);
+
+	if (offset > bufsize || bufsize - offset < min_len)
+		return false;
+
+	len = isonum_711(de->length);
+	name_len = isonum_711(de->name_len);
+	if (len < min_len || name_len > len - min_len)
+		return false;
+	if (len > bufsize - offset)
+		return false;
+	return true;
+}
+
 static struct dentry *
 isofs_export_iget(struct super_block *sb,
 		  unsigned long block,
@@ -83,13 +103,21 @@ static struct dentry *isofs_export_get_parent(struct dentry *child)
 
 	/* This is the "." entry. */
 	de = (struct iso_directory_record*)bh->b_data;
+	if (!isofs_dir_record_valid(de, 0, child_inode->i_sb->s_blocksize) ||
+	    isonum_711(de->name_len) != 1 || de->name[0] != 0) {
+		printk(KERN_ERR "isofs: Unable to find the \".\" directory for NFS.\n");
+		rv = ERR_PTR(-EACCES);
+		goto out;
+	}
 
 	/* The ".." entry is always the second entry. */
 	parent_offset = (unsigned long)isonum_711(de->length);
 	de = (struct iso_directory_record*)(bh->b_data + parent_offset);
 
 	/* Verify it is in fact the ".." entry. */
-	if ((isonum_711(de->name_len) != 1) || (de->name[0] != 1)) {
+	if (!isofs_dir_record_valid(de, parent_offset,
+				    child_inode->i_sb->s_blocksize) ||
+	    isonum_711(de->name_len) != 1 || de->name[0] != 1) {
 		printk(KERN_ERR "isofs: Unable to find the \"..\" "
 		       "directory for NFS.\n");
 		rv = ERR_PTR(-EACCES);
-- 
2.51.0

Re: [PATCH] isofs: validate directory records in NFS get_parent

[Jan Kara]

On Tue 21-07-26 18:13:18, Yichong Chen wrote:
> isofs_export_get_parent() assumes valid "." and ".." entries.
> 
> A malformed image can provide an invalid length for the first entry.
> 
> Validate both records before using the first length as the ".." offset.
> 
> This keeps the NFS export get_parent path from accepting malformed records.
> 
> Signed-off-by: Yichong Chen <chenyichong@uniontech.com>

This looks sensible. But for maintainability, can you please move
isofs_dir_record_valid() to namei.c and make sure readdir and directory
lookup functions are using this function for directory entry checking as
well (and remove the duplicit checks there)? So that we have the same
checks everywhere... Thanks.

								Honza

> ---
>  fs/isofs/export.c | 30 +++++++++++++++++++++++++++++-
>  1 file changed, 29 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/isofs/export.c b/fs/isofs/export.c
> index 78f80c1a5c54..bffec45bb274 100644
> --- a/fs/isofs/export.c
> +++ b/fs/isofs/export.c
> @@ -16,6 +16,26 @@
>  
>  #include "isofs.h"
>  
> +static bool isofs_dir_record_valid(struct iso_directory_record *de,
> +				   unsigned long offset,
> +				   unsigned long bufsize)
> +{
> +	unsigned int len;
> +	unsigned int name_len;
> +	unsigned long min_len = offsetof(struct iso_directory_record, name);
> +
> +	if (offset > bufsize || bufsize - offset < min_len)
> +		return false;
> +
> +	len = isonum_711(de->length);
> +	name_len = isonum_711(de->name_len);
> +	if (len < min_len || name_len > len - min_len)
> +		return false;
> +	if (len > bufsize - offset)
> +		return false;
> +	return true;
> +}
> +
>  static struct dentry *
>  isofs_export_iget(struct super_block *sb,
>  		  unsigned long block,
> @@ -83,13 +103,21 @@ static struct dentry *isofs_export_get_parent(struct dentry *child)
>  
>  	/* This is the "." entry. */
>  	de = (struct iso_directory_record*)bh->b_data;
> +	if (!isofs_dir_record_valid(de, 0, child_inode->i_sb->s_blocksize) ||
> +	    isonum_711(de->name_len) != 1 || de->name[0] != 0) {
> +		printk(KERN_ERR "isofs: Unable to find the \".\" directory for NFS.\n");
> +		rv = ERR_PTR(-EACCES);
> +		goto out;
> +	}
>  
>  	/* The ".." entry is always the second entry. */
>  	parent_offset = (unsigned long)isonum_711(de->length);
>  	de = (struct iso_directory_record*)(bh->b_data + parent_offset);
>  
>  	/* Verify it is in fact the ".." entry. */
> -	if ((isonum_711(de->name_len) != 1) || (de->name[0] != 1)) {
> +	if (!isofs_dir_record_valid(de, parent_offset,
> +				    child_inode->i_sb->s_blocksize) ||
> +	    isonum_711(de->name_len) != 1 || de->name[0] != 1) {
>  		printk(KERN_ERR "isofs: Unable to find the \"..\" "
>  		       "directory for NFS.\n");
>  		rv = ERR_PTR(-EACCES);
> -- 
> 2.51.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH] isofs: validate directory records in NFS get_parent

[Yichong Chen]

Thanks for the review. I'll rework it as suggested and send a v2.

Yichong

[PATCH v2] isofs: validate directory records consistently

[Yichong Chen]

isofs_export_get_parent() assumes that the first two directory records
are valid "." and ".." entries.  A malformed image can provide an
invalid length for the first entry, causing the computed ".." offset to
point outside the received block.

Add a shared directory record validator and use it in NFS get_parent,
readdir and lookup.  This keeps the basic directory record length checks
consistent across all directory users before they consume the name field
or use one record length to find the next entry.

Signed-off-by: Yichong Chen <chenyichong@uniontech.com>
---
v2:
- Move the directory record validator to namei.c.
- Share the validator with NFS get_parent, readdir and lookup.
- Replace duplicate directory record length checks.

 fs/isofs/dir.c    |  7 ++-----
 fs/isofs/export.c | 10 +++++++++-
 fs/isofs/isofs.h  |  3 +++
 fs/isofs/namei.c  | 28 ++++++++++++++++++++++++----
 4 files changed, 38 insertions(+), 10 deletions(-)

diff --git a/fs/isofs/dir.c b/fs/isofs/dir.c
index cc587cd25162..a96268c9ca41 100644
--- a/fs/isofs/dir.c
+++ b/fs/isofs/dir.c
@@ -149,10 +149,8 @@ static int do_isofs_readdir(struct inode *inode, struct file *file,
 			}
 			de = tmpde;
 		}
-		/* Basic sanity check, whether name doesn't exceed dir entry */
-		if (de_len < sizeof(struct iso_directory_record) ||
-		    de_len < de->name_len[0] +
-					sizeof(struct iso_directory_record)) {
+		if (!isofs_dir_record_valid(de, de == tmpde ? 0 : offset_saved,
+					    de == tmpde ? de_len : bufsize)) {
 			printk(KERN_NOTICE "iso9660: Corrupted directory entry"
 			       " in block %lu of inode %llu\n", block,
 			       inode->i_ino);
@@ -300,4 +298,3 @@ const struct inode_operations isofs_dir_inode_operations =
 	.fileattr_get = isofs_fileattr_get,
 };
 
-
diff --git a/fs/isofs/export.c b/fs/isofs/export.c
index 78f80c1a5c54..4f7fa1d508a1 100644
--- a/fs/isofs/export.c
+++ b/fs/isofs/export.c
@@ -83,13 +83,21 @@ static struct dentry *isofs_export_get_parent(struct dentry *child)
 
 	/* This is the "." entry. */
 	de = (struct iso_directory_record*)bh->b_data;
+	if (!isofs_dir_record_valid(de, 0, child_inode->i_sb->s_blocksize) ||
+	    isonum_711(de->name_len) != 1 || de->name[0] != 0) {
+		printk(KERN_ERR "isofs: Unable to find the \".\" directory for NFS.\n");
+		rv = ERR_PTR(-EACCES);
+		goto out;
+	}
 
 	/* The ".." entry is always the second entry. */
 	parent_offset = (unsigned long)isonum_711(de->length);
 	de = (struct iso_directory_record*)(bh->b_data + parent_offset);
 
 	/* Verify it is in fact the ".." entry. */
-	if ((isonum_711(de->name_len) != 1) || (de->name[0] != 1)) {
+	if (!isofs_dir_record_valid(de, parent_offset,
+				    child_inode->i_sb->s_blocksize) ||
+	    isonum_711(de->name_len) != 1 || de->name[0] != 1) {
 		printk(KERN_ERR "isofs: Unable to find the \"..\" "
 		       "directory for NFS.\n");
 		rv = ERR_PTR(-EACCES);
diff --git a/fs/isofs/isofs.h b/fs/isofs/isofs.h
index 0ec8b24a42ed..dacb9cdae4fd 100644
--- a/fs/isofs/isofs.h
+++ b/fs/isofs/isofs.h
@@ -115,6 +115,9 @@ struct inode;		/* To make gcc happy */
 extern int parse_rock_ridge_inode(struct iso_directory_record *, struct inode *, int relocated);
 extern int get_rock_ridge_filename(struct iso_directory_record *, char *, struct inode *);
 extern int isofs_name_translate(struct iso_directory_record *, char *, struct inode *);
+bool isofs_dir_record_valid(struct iso_directory_record *de,
+			    unsigned long offset,
+			    unsigned long bufsize);
 
 int get_joliet_filename(struct iso_directory_record *, unsigned char *, struct inode *);
 int get_acorn_filename(struct iso_directory_record *, char *, struct inode *);
diff --git a/fs/isofs/namei.c b/fs/isofs/namei.c
index 3ace3d6a55e7..a161b28893d6 100644
--- a/fs/isofs/namei.c
+++ b/fs/isofs/namei.c
@@ -10,6 +10,26 @@
 #include <linux/gfp.h>
 #include "isofs.h"
 
+bool isofs_dir_record_valid(struct iso_directory_record *de,
+			    unsigned long offset,
+			    unsigned long bufsize)
+{
+	unsigned int len;
+	unsigned int name_len;
+	unsigned long min_len = offsetof(struct iso_directory_record, name);
+
+	if (offset > bufsize || bufsize - offset < min_len)
+		return false;
+
+	len = isonum_711(de->length);
+	name_len = isonum_711(de->name_len);
+	if (len < min_len || name_len > len - min_len)
+		return false;
+	if (len > bufsize - offset)
+		return false;
+	return true;
+}
+
 static int
 isofs_cmp(struct dentry *dentry, const char *compare, int dlen)
 {
@@ -88,16 +108,16 @@ isofs_find_entry(struct inode *dir, struct dentry *dentry,
 			de = tmpde;
 		}
 
-		dlen = de->name_len[0];
-		dpnt = de->name;
-		/* Basic sanity check, whether name doesn't exceed dir entry */
-		if (de_len < dlen + sizeof(struct iso_directory_record)) {
+		if (!isofs_dir_record_valid(de, de == tmpde ? 0 : offset_saved,
+					    de == tmpde ? de_len : bufsize)) {
 			printk(KERN_NOTICE "iso9660: Corrupted directory entry"
 			       " in block %lu of inode %llu\n", block,
 			       dir->i_ino);
 			brelse(bh);
 			return 0;
 		}
+		dlen = de->name_len[0];
+		dpnt = de->name;
 
 		if (sbi->s_rock &&
 		    ((i = get_rock_ridge_filename(de, tmpname, dir)))) {
-- 
2.51.0

Re: [PATCH v2] isofs: validate directory records consistently

[Jan Kara]

On Tue 28-07-26 15:43:49, Yichong Chen wrote:
> isofs_export_get_parent() assumes that the first two directory records
> are valid "." and ".." entries.  A malformed image can provide an
> invalid length for the first entry, causing the computed ".." offset to
> point outside the received block.
> 
> Add a shared directory record validator and use it in NFS get_parent,
> readdir and lookup.  This keeps the basic directory record length checks
> consistent across all directory users before they consume the name field
> or use one record length to find the next entry.
> 
> Signed-off-by: Yichong Chen <chenyichong@uniontech.com>

Thanks! I've merged this patch to my tree.

								Honza

> ---
> v2:
> - Move the directory record validator to namei.c.
> - Share the validator with NFS get_parent, readdir and lookup.
> - Replace duplicate directory record length checks.
> 
>  fs/isofs/dir.c    |  7 ++-----
>  fs/isofs/export.c | 10 +++++++++-
>  fs/isofs/isofs.h  |  3 +++
>  fs/isofs/namei.c  | 28 ++++++++++++++++++++++++----
>  4 files changed, 38 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/isofs/dir.c b/fs/isofs/dir.c
> index cc587cd25162..a96268c9ca41 100644
> --- a/fs/isofs/dir.c
> +++ b/fs/isofs/dir.c
> @@ -149,10 +149,8 @@ static int do_isofs_readdir(struct inode *inode, struct file *file,
>  			}
>  			de = tmpde;
>  		}
> -		/* Basic sanity check, whether name doesn't exceed dir entry */
> -		if (de_len < sizeof(struct iso_directory_record) ||
> -		    de_len < de->name_len[0] +
> -					sizeof(struct iso_directory_record)) {
> +		if (!isofs_dir_record_valid(de, de == tmpde ? 0 : offset_saved,
> +					    de == tmpde ? de_len : bufsize)) {
>  			printk(KERN_NOTICE "iso9660: Corrupted directory entry"
>  			       " in block %lu of inode %llu\n", block,
>  			       inode->i_ino);
> @@ -300,4 +298,3 @@ const struct inode_operations isofs_dir_inode_operations =
>  	.fileattr_get = isofs_fileattr_get,
>  };
>  
> -
> diff --git a/fs/isofs/export.c b/fs/isofs/export.c
> index 78f80c1a5c54..4f7fa1d508a1 100644
> --- a/fs/isofs/export.c
> +++ b/fs/isofs/export.c
> @@ -83,13 +83,21 @@ static struct dentry *isofs_export_get_parent(struct dentry *child)
>  
>  	/* This is the "." entry. */
>  	de = (struct iso_directory_record*)bh->b_data;
> +	if (!isofs_dir_record_valid(de, 0, child_inode->i_sb->s_blocksize) ||
> +	    isonum_711(de->name_len) != 1 || de->name[0] != 0) {
> +		printk(KERN_ERR "isofs: Unable to find the \".\" directory for NFS.\n");
> +		rv = ERR_PTR(-EACCES);
> +		goto out;
> +	}
>  
>  	/* The ".." entry is always the second entry. */
>  	parent_offset = (unsigned long)isonum_711(de->length);
>  	de = (struct iso_directory_record*)(bh->b_data + parent_offset);
>  
>  	/* Verify it is in fact the ".." entry. */
> -	if ((isonum_711(de->name_len) != 1) || (de->name[0] != 1)) {
> +	if (!isofs_dir_record_valid(de, parent_offset,
> +				    child_inode->i_sb->s_blocksize) ||
> +	    isonum_711(de->name_len) != 1 || de->name[0] != 1) {
>  		printk(KERN_ERR "isofs: Unable to find the \"..\" "
>  		       "directory for NFS.\n");
>  		rv = ERR_PTR(-EACCES);
> diff --git a/fs/isofs/isofs.h b/fs/isofs/isofs.h
> index 0ec8b24a42ed..dacb9cdae4fd 100644
> --- a/fs/isofs/isofs.h
> +++ b/fs/isofs/isofs.h
> @@ -115,6 +115,9 @@ struct inode;		/* To make gcc happy */
>  extern int parse_rock_ridge_inode(struct iso_directory_record *, struct inode *, int relocated);
>  extern int get_rock_ridge_filename(struct iso_directory_record *, char *, struct inode *);
>  extern int isofs_name_translate(struct iso_directory_record *, char *, struct inode *);
> +bool isofs_dir_record_valid(struct iso_directory_record *de,
> +			    unsigned long offset,
> +			    unsigned long bufsize);
>  
>  int get_joliet_filename(struct iso_directory_record *, unsigned char *, struct inode *);
>  int get_acorn_filename(struct iso_directory_record *, char *, struct inode *);
> diff --git a/fs/isofs/namei.c b/fs/isofs/namei.c
> index 3ace3d6a55e7..a161b28893d6 100644
> --- a/fs/isofs/namei.c
> +++ b/fs/isofs/namei.c
> @@ -10,6 +10,26 @@
>  #include <linux/gfp.h>
>  #include "isofs.h"
>  
> +bool isofs_dir_record_valid(struct iso_directory_record *de,
> +			    unsigned long offset,
> +			    unsigned long bufsize)
> +{
> +	unsigned int len;
> +	unsigned int name_len;
> +	unsigned long min_len = offsetof(struct iso_directory_record, name);
> +
> +	if (offset > bufsize || bufsize - offset < min_len)
> +		return false;
> +
> +	len = isonum_711(de->length);
> +	name_len = isonum_711(de->name_len);
> +	if (len < min_len || name_len > len - min_len)
> +		return false;
> +	if (len > bufsize - offset)
> +		return false;
> +	return true;
> +}
> +
>  static int
>  isofs_cmp(struct dentry *dentry, const char *compare, int dlen)
>  {
> @@ -88,16 +108,16 @@ isofs_find_entry(struct inode *dir, struct dentry *dentry,
>  			de = tmpde;
>  		}
>  
> -		dlen = de->name_len[0];
> -		dpnt = de->name;
> -		/* Basic sanity check, whether name doesn't exceed dir entry */
> -		if (de_len < dlen + sizeof(struct iso_directory_record)) {
> +		if (!isofs_dir_record_valid(de, de == tmpde ? 0 : offset_saved,
> +					    de == tmpde ? de_len : bufsize)) {
>  			printk(KERN_NOTICE "iso9660: Corrupted directory entry"
>  			       " in block %lu of inode %llu\n", block,
>  			       dir->i_ino);
>  			brelse(bh);
>  			return 0;
>  		}
> +		dlen = de->name_len[0];
> +		dpnt = de->name;
>  
>  		if (sbi->s_rock &&
>  		    ((i = get_rock_ridge_filename(de, tmpname, dir)))) {
> -- 
> 2.51.0
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR