linux-kernel - Re: [PATCH 1/1] hfsplus: fix longname handling

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 25 Feb 2014 11:37:25 +0400
From:	Vyacheslav Dubeyko <slava@...eyko.com>
To:	sougata@...era.com
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	Joe Perches <joe@...ches.com>, Christoph Hellwig <hch@....de>,
	Al Viro <viro@...iv.linux.org.uk>,
	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/1] hfsplus: fix longname handling

Hi Sougata,

On Mon, 2014-02-24 at 21:28 +0200, Sougata Santra wrote:
> -ENAMETOOLONG returned from hfsplus_asc2uni was not propaged to iops. This
> allowed hfsplus to create files/directories with HFSPLUS_MAX_STRLEN and
> incorrect keys, leaving the FS in an inconsistent state. This patch fixes
> this issue.
> 

Good fix. Thank you.

I have some small remarks. Please, see below.

> Signed-off-by: Sougata Santra <sougata@...era.com>
> Reviewed-by: Christoph Hellwig <hch@....de>
> ---
>  fs/hfsplus/catalog.c    | 89 ++++++++++++++++++++++++++++++++++++-------------
>  fs/hfsplus/dir.c        | 11 ++++--
>  fs/hfsplus/hfsplus_fs.h |  4 ++-
>  fs/hfsplus/super.c      |  4 ++-
>  4 files changed, 79 insertions(+), 29 deletions(-)
> 
> diff --git a/fs/hfsplus/catalog.c b/fs/hfsplus/catalog.c
> index 968ce41..389c474 100644
> --- a/fs/hfsplus/catalog.c
> +++ b/fs/hfsplus/catalog.c
> @@ -38,21 +38,30 @@ int hfsplus_cat_bin_cmp_key(const hfsplus_btree_key *k1,
>  	return hfsplus_strcmp(&k1->cat.name, &k2->cat.name);
>  }
>  
> -void hfsplus_cat_build_key(struct super_block *sb, hfsplus_btree_key *key,
> -			   u32 parent, struct qstr *str)
> +/* Generates key for catalog file/folders record. */
> +int hfsplus_cat_build_key(struct super_block *sb,
> +		hfsplus_btree_key *key, u32 parent, struct qstr *str)
>  {
> -	int len;
> +	int len, err;
>  
>  	key->cat.parent = cpu_to_be32(parent);
> -	if (str) {
> -		hfsplus_asc2uni(sb, &key->cat.name, HFSPLUS_MAX_STRLEN,
> -					str->name, str->len);
> -		len = be16_to_cpu(key->cat.name.length);
> -	} else {
> -		key->cat.name.length = 0;
> -		len = 0;
> -	}
> +	err = hfsplus_asc2uni(sb, &key->cat.name, HFSPLUS_MAX_STRLEN,
> +			str->name, str->len);
> +	if (unlikely(err < 0))
> +		return err;
> +
> +	len = be16_to_cpu(key->cat.name.length);
>  	key->key_len = cpu_to_be16(6 + 2 * len);

I think that maybe it is time to change hardcoded 6 on sensible named
constant. What do you think?

> +	return 0;
> +}
> +
> +/* Generates key for catalog thread record. */
> +void hfsplus_cat_build_key_with_cnid(struct super_block *sb,
> +			hfsplus_btree_key *key, u32 parent)
> +{
> +	key->cat.parent = cpu_to_be32(parent);
> +	key->cat.name.length = 0;
> +	key->key_len = cpu_to_be16(6);

Ditto.

>  }
>  
>  static void hfsplus_cat_build_key_uni(hfsplus_btree_key *key, u32 parent,

We have such code for hfsplus_cat_build_key_uni():

 58 static void hfsplus_cat_build_key_uni(hfsplus_btree_key *key, u32 parent,
 59                                       struct hfsplus_unistr *name)
 60 {
 61         int ustrlen;
 62 
 63         ustrlen = be16_to_cpu(name->length);

I suppose that it makes sense to check name->length here and return
error. We can check possible volume corruption here.

 64         key->cat.parent = cpu_to_be32(parent);
 65         key->cat.name.length = cpu_to_be16(ustrlen);
 66         ustrlen *= 2;
 67         memcpy(key->cat.name.unicode, name->unicode, ustrlen);
 68         key->key_len = cpu_to_be16(6 + ustrlen);
 69 }

What do you think about such suggestion?

> diff --git a/fs/hfsplus/hfsplus_fs.h b/fs/hfsplus/hfsplus_fs.h
> index 08846425b..66671c4 100644
> --- a/fs/hfsplus/hfsplus_fs.h
> +++ b/fs/hfsplus/hfsplus_fs.h
> @@ -443,8 +443,10 @@ int hfsplus_cat_case_cmp_key(const hfsplus_btree_key *,
>  		const hfsplus_btree_key *);
>  int hfsplus_cat_bin_cmp_key(const hfsplus_btree_key *,
>  		const hfsplus_btree_key *);
> -void hfsplus_cat_build_key(struct super_block *sb,
> +int hfsplus_cat_build_key(struct super_block *sb,
>  		hfsplus_btree_key *, u32, struct qstr *);
> +void hfsplus_cat_build_key_with_cnid(struct super_block *sb,

The whole style of the fix is good. But such mess looks not very good.
But it is not critical, of course. :)

Thanks,
Vyacheslav Dubeyko.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/