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@xxxxxxxxxxxxx>

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@xxxxxxxx>
SUSE Labs, CR