Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Hawkins Jiawei <yin31149@gmail.com>
To:     chenxiaosong2@huawei.com
Cc:     akpm@linux-foundation.org, anton@tuxera.com,
        linux-kernel@vger.kernel.org, linux-ntfs-dev@lists.sourceforge.net,
        liuyongqiang13@huawei.com, yi.zhang@huawei.com,
        zhangxiaoxu5@huawei.com, 18801353760@163.com,
        skhan@linuxfoundation.org, paskripkin@gmail.com,
        linux-kernel-mentees@lists.linuxfoundation.org
Subject: Re: [PATCH v2] ntfs: fix use-after-free in ntfs_ucsncmp()
Date:   Sat,  9 Jul 2022 08:54:51 +0800
Message-Id: <20220709005450.2220-1-yin31149@gmail.com>
In-Reply-To: <20220707105329.4020708-1-chenxiaosong2@huawei.com>
References: <20220707105329.4020708-1-chenxiaosong2@huawei.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

> Fix this by adding sanity check on end address of attibute name string.

> diff --git a/fs/ntfs/attrib.c b/fs/ntfs/attrib.c
> index 4de597a83b88..9ab8766872d2 100644
> --- a/fs/ntfs/attrib.c
> +++ b/fs/ntfs/attrib.c
> @@ -592,8 +592,12 @@ static int ntfs_attr_find(const ATTR_TYPE type, const ntfschar *name,
>  		a = (ATTR_RECORD*)((u8*)ctx->attr +
>  				le32_to_cpu(ctx->attr->length));
>  	for (;;	a = (ATTR_RECORD*)((u8*)a + le32_to_cpu(a->length))) {
> -		if ((u8*)a < (u8*)ctx->mrec || (u8*)a > (u8*)ctx->mrec +
> -				le32_to_cpu(ctx->mrec->bytes_allocated))
> +		u8 *mrec_end = (u8 *)ctx->mrec +
> +		               le32_to_cpu(ctx->mrec->bytes_allocated);
> +		u8 *name_end = (u8 *)a + le16_to_cpu(a->name_offset) +
> +		               a->name_length;

In my opinion, name_length field just means the number of characters,
yet each character is a ntfschar type. So name should be
name_length * sizeof(ntfschar) bytes. The example is at
https://elixir.bootlin.com/linux/v5.19-rc5/source/fs/ntfs/attrib.c#L1667

> +		if ((u8*)a < (u8*)ctx->mrec || (u8*)a > mrec_end ||
> +		    name_end > mrec_end)
>  			break;
>  		ctx->attr = a;
>  		if (unlikely(le32_to_cpu(a->type) > le32_to_cpu(type) ||
> --