Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
To:     Matteo Croce <mcroce@redhat.com>
Cc:     Timothy Redaelli <tredaelli@redhat.com>,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] vfat: don't read garbage after last dirent
References: <20181207013410.7050-1-mcroce@redhat.com>
Date:   Fri, 14 Dec 2018 12:41:15 +0900
In-Reply-To: <20181207013410.7050-1-mcroce@redhat.com> (Matteo Croce's message
        of "Fri, 7 Dec 2018 02:34:10 +0100")
Message-ID: <87lg4sdbc4.fsf@mail.parknet.co.jp>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.0.50 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Matteo Croce <mcroce@redhat.com> writes:

> The FAT32 File System Specification[1] states:
>
>     If DIR_Name[0] == 0x00, then the directory entry is free, and there
>     are no allocated directory entries after this one.
>
>     The special 0 value, indicates to FAT file system driver code that
>     the rest of the entries in this directory do notneed to be examined
>     because they are all free.
>
> This is not enforced by Linux, and is possible to read garbage if
> not all the dirents after the last one are filled with zeroes.
>
> [1] http://download.microsoft.com/download/1/6/1/161ba512-40e2-4cc9-843a-923143f3456c/fatgen103.doc
>
> Reported-by: Timothy Redaelli <tredaelli@redhat.com>
> Signed-off-by: Matteo Croce <mcroce@redhat.com>

I know this spec. But name[0] == 0 means - the rest of the entries in
this directory must be 0. Otherwise, overwriting zeroed entry by adding
new dir entry, then shows garbage as dir entry. So "stop at zero" is
just a optimization.

On other hand, I know there is buggy formatters don't clear that
garbage, and uses it on read-only storage area. 

So I will agree to supporting "stop at zero" though (and keep assuming
dir is initialized with zero clear. i.e. don't add tricky workaround to
support buggy formatters.), the implementation should handle it for
whole path, not only readdir().

E.g. I recall lookup, dir empty check path, and maybe there are others I
can't recall immediately.

>  get_new:
> -	if (fat_get_entry(inode, &cpos, &bh, &de) == -1)
> +	if (fat_get_entry(inode, &cpos, &bh, &de) == -1 || !de->name[0])
>  		goto end_of_dir;
>  parse_record:
>  	nr_slots = 0;

Thanks.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>