On Sun, 2025-09-07 at 09:08 +0800, k.chen wrote:
> The previous fix (94458781aee6) was insufficient,
> as it did not consider that
> sizeof(struct hfsplus_attr_unistr) != sizeof(struct hfsplus_unistr).
Could you please explain in more details what is issue here and how have you
fixed the issue?
Currently, it is really hard to follow to the commit message.
>
> Fixes: 94458781aee6 ("hfsplus: fix slab-out-of-bounds read in hfsplus_uni2asc()")
> Signed-off-by: k.chen <k.chen@xxxxxxxxxxxxxxxx>
It will be really great if you can share your full name.
> ---
> V2 -> V1: change struct pointer type to pass compiler
>
> fs/hfsplus/dir.c | 3 ++-
> fs/hfsplus/hfsplus_fs.h | 2 +-
> fs/hfsplus/unicode.c | 9 ++++-----
> fs/hfsplus/xattr.c | 6 ++++--
> 4 files changed, 11 insertions(+), 9 deletions(-)
>
> diff --git a/fs/hfsplus/dir.c b/fs/hfsplus/dir.c
> index 876bbb80fb4d..765627fc5ebe 100644
> --- a/fs/hfsplus/dir.c
> +++ b/fs/hfsplus/dir.c
> @@ -204,7 +204,8 @@ static int hfsplus_readdir(struct file *file, struct dir_context *ctx)
> fd.entrylength);
> type = be16_to_cpu(entry.type);
> len = NLS_MAX_CHARSET_SIZE * HFSPLUS_MAX_STRLEN;
> - err = hfsplus_uni2asc(sb, &fd.key->cat.name, strbuf, &len);
> + err = hfsplus_uni2asc(sb, &fd.key->cat.name, HFSPLUS_MAX_STRLEN,
Probably, it makes sense to introduce a constant to keep one-line call.
Why not declare above likewise constant?
const size_t max_len = HFSPLUS_MAX_STRLEN;
> + strbuf, &len);
> if (err)
> goto out;
> if (type == HFSPLUS_FOLDER) {
> diff --git a/fs/hfsplus/hfsplus_fs.h b/fs/hfsplus/hfsplus_fs.h
> index 96a5c24813dd..49d97c46fd0a 100644
> --- a/fs/hfsplus/hfsplus_fs.h
> +++ b/fs/hfsplus/hfsplus_fs.h
> @@ -522,7 +522,7 @@ int hfsplus_strcasecmp(const struct hfsplus_unistr *s1,
> int hfsplus_strcmp(const struct hfsplus_unistr *s1,
> const struct hfsplus_unistr *s2);
> int hfsplus_uni2asc(struct super_block *sb, const struct hfsplus_unistr *ustr,
> - char *astr, int *len_p);
> + int max_unistr_len, char *astr, int *len_p);
max_len could be enough, for my taste.
> int hfsplus_asc2uni(struct super_block *sb, struct hfsplus_unistr *ustr,
> int max_unistr_len, const char *astr, int len);
> int hfsplus_hash_dentry(const struct dentry *dentry, struct qstr *str);
> diff --git a/fs/hfsplus/unicode.c b/fs/hfsplus/unicode.c
> index 36b6cf2a3abb..b4303785ba1e 100644
> --- a/fs/hfsplus/unicode.c
> +++ b/fs/hfsplus/unicode.c
> @@ -119,9 +119,8 @@ static u16 *hfsplus_compose_lookup(u16 *p, u16 cc)
> return NULL;
> }
>
> -int hfsplus_uni2asc(struct super_block *sb,
> - const struct hfsplus_unistr *ustr,
> - char *astr, int *len_p)
> +int hfsplus_uni2asc(struct super_block *sb, const struct hfsplus_unistr *ustr,
> + int max_unistr_len, char *astr, int *len_p)
Another possible way is to make hfsplus_uni2asc() by static function and to
introduce two inline functions hfsplus_uni2asc_str(),
hfsplus_uni2asc_xattr_str(). What do you think?
> {
> const hfsplus_unichr *ip;
> struct nls_table *nls = HFSPLUS_SB(sb)->nls;
> @@ -134,8 +133,8 @@ int hfsplus_uni2asc(struct super_block *sb,
> ip = ustr->unicode;
>
> ustrlen = be16_to_cpu(ustr->length);
> - if (ustrlen > HFSPLUS_MAX_STRLEN) {
> - ustrlen = HFSPLUS_MAX_STRLEN;
> + if (ustrlen > max_unistr_len) {
> + ustrlen = max_unistr_len;
> pr_err("invalid length %u has been corrected to %d\n",
> be16_to_cpu(ustr->length), ustrlen);
> }
> diff --git a/fs/hfsplus/xattr.c b/fs/hfsplus/xattr.c
> index 18dc3d254d21..456c7d6b2356 100644
> --- a/fs/hfsplus/xattr.c
> +++ b/fs/hfsplus/xattr.c
> @@ -736,8 +736,10 @@ ssize_t hfsplus_listxattr(struct dentry *dentry, char *buffer, size_t size)
>
> xattr_name_len = NLS_MAX_CHARSET_SIZE * HFSPLUS_ATTR_MAX_STRLEN;
> if (hfsplus_uni2asc(inode->i_sb,
> - (const struct hfsplus_unistr *)&fd.key->attr.key_name,
> - strbuf, &xattr_name_len)) {
> + (const struct hfsplus_unistr *)&fd.key->attr
> + .key_name,
> + HFSPLUS_ATTR_MAX_STRLEN, strbuf,
> + &xattr_name_len)) {
Frankly speaking, it looks like a mess. Could we make it more compact and
cleaner? Could you please rework it?
Thanks,
Slava.
> pr_err("unicode conversion failed\n");
> res = -EIO;
> goto end_listxattr;
