[PATCH v2] vfat: don't read garbage after last dirent

[PATCH v2] vfat: don't read garbage after last dirent

[Matteo Croce]

The FAT32 File System Specification[1] states that:

    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 not need to be examined
    because they are all free.

This is not enforced by Linux, and is possible to read garbage if not
all 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>
---
v2:
* add the check also in lookup and dir empty check
* fix two tipos in the commit message  

 fs/fat/dir.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/fs/fat/dir.c b/fs/fat/dir.c
index c8366cb8eccd..955edf5df286 100644
--- a/fs/fat/dir.c
+++ b/fs/fat/dir.c
@@ -588,7 +588,7 @@ static int __fat_readdir(struct inode *inode, struct file *file,
 
 	bh = NULL;
 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;
@@ -916,7 +916,8 @@ int fat_dir_empty(struct inode *dir)
 
 	bh = NULL;
 	cpos = 0;
-	while (fat_get_short_entry(dir, &cpos, &bh, &de) >= 0) {
+	while (fat_get_short_entry(dir, &cpos, &bh, &de) >= 0 &&
+	       de->name[0]) {
 		if (strncmp(de->name, MSDOS_DOT   , MSDOS_NAME) &&
 		    strncmp(de->name, MSDOS_DOTDOT, MSDOS_NAME)) {
 			result = -ENOTEMPTY;
@@ -961,7 +962,7 @@ int fat_scan(struct inode *dir, const unsigned char *name,
 	sinfo->slot_off = 0;
 	sinfo->bh = NULL;
 	while (fat_get_short_entry(dir, &sinfo->slot_off, &sinfo->bh,
-				   &sinfo->de) >= 0) {
+				   &sinfo->de) >= 0 && sinfo->de->name[0]) {
 		if (!strncmp(sinfo->de->name, name, MSDOS_NAME)) {
 			sinfo->slot_off -= sizeof(*sinfo->de);
 			sinfo->nr_slots = 1;
-- 
2.19.2

Re: [PATCH v2] vfat: don't read garbage after last dirent

[OGAWA Hirofumi]

Matteo Croce <mcroce@redhat.com> writes:

> The FAT32 File System Specification[1] states that:
>
>     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 not need to be examined
>     because they are all free.
>
> This is not enforced by Linux, and is possible to read garbage if not
> all 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>

We have to handle all paths that is using fat_get_entry(), to make
consistent behavior.

With quick check, there are still several issues remaining. Please check
more. For example, looks like fat_parse_long()/fat_search_long() path is
missing, and fat_get_dotdot_entry(), fat_subdirs() too.

(while adding new entry, if we found zeroed entry, we would be better to
warn about fsck.)

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

Re: [PATCH v2] vfat: don't read garbage after last dirent

[Matteo Croce]

On Mon, Dec 24, 2018 at 1:14 PM OGAWA Hirofumi
<hirofumi@mail.parknet.co.jp> wrote:
>
> Matteo Croce <mcroce@redhat.com> writes:
>
> > The FAT32 File System Specification[1] states that:
> >
> >     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 not need to be examined
> >     because they are all free.
> >
> > This is not enforced by Linux, and is possible to read garbage if not
> > all 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>
>
> We have to handle all paths that is using fat_get_entry(), to make
> consistent behavior.
>
> With quick check, there are still several issues remaining. Please check
> more. For example, looks like fat_parse_long()/fat_search_long() path is
> missing, and fat_get_dotdot_entry(), fat_subdirs() too.
>

If I put the check in fat_get_short_entry(), then
fat_get_dotdot_entry() and fat_subdirs() are covered too.
Is there any drawback in doing this?

> (while adding new entry, if we found zeroed entry, we would be better to
> warn about fsck.)
>

Ok

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

Thanks,
-- 
Matteo Croce
per aspera ad upstream