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

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