Linux-Fsdevel Archive on lore.kernel.org
 help / Atom feed
* [PATCH v3] vfat: don't read garbage after last dirent
@ 2019-02-01  0:14 Matteo Croce
  2019-02-13 17:29 ` OGAWA Hirofumi
  0 siblings, 1 reply; 2+ messages in thread
From: Matteo Croce @ 2019-02-01  0:14 UTC (permalink / raw)
  To: linux-kernel, linux-fsdevel; +Cc: OGAWA Hirofumi, Timothy Redaelli

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>
---
 fs/fat/dir.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/fs/fat/dir.c b/fs/fat/dir.c
index 9d01db37183f..d919a1ee519c 100644
--- a/fs/fat/dir.c
+++ b/fs/fat/dir.c
@@ -314,7 +314,7 @@ static int fat_parse_long(struct inode *dir, loff_t *pos,
 
 		if (ds->id & 0x40)
 			(*unicode)[offset + 13] = 0;
-		if (fat_get_entry(dir, pos, bh, de) < 0)
+		if (fat_get_entry(dir, pos, bh, de) < 0 || !(*de)->name[0])
 			return PARSE_EOF;
 		if (slot == 0)
 			break;
@@ -476,7 +476,8 @@ int fat_search_long(struct inode *inode, const unsigned char *name,
 
 	err = -ENOENT;
 	while (1) {
-		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;
@@ -588,7 +589,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;
@@ -898,7 +899,8 @@ int fat_get_dotdot_entry(struct inode *dir, struct buffer_head **bh,
 	loff_t offset = 0;
 
 	*de = NULL;
-	while (fat_get_short_entry(dir, &offset, bh, de) >= 0) {
+	while (fat_get_short_entry(dir, &offset, bh, de) >= 0 &&
+	       (*de)->name[0]) {
 		if (!strncmp((*de)->name, MSDOS_DOTDOT, MSDOS_NAME))
 			return 0;
 	}
@@ -916,7 +918,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;
@@ -941,7 +944,8 @@ int fat_subdirs(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 (de->attr & ATTR_DIR)
 			count++;
 	}
@@ -961,7 +965,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;
@@ -985,7 +989,7 @@ int fat_scan_logstart(struct inode *dir, int i_logstart,
 	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 (fat_get_start(MSDOS_SB(sb), sinfo->de) == i_logstart) {
 			sinfo->slot_off -= sizeof(*sinfo->de);
 			sinfo->nr_slots = 1;
-- 
2.20.1


^ permalink raw reply	[flat|nested] 2+ messages in thread

* Re: [PATCH v3] vfat: don't read garbage after last dirent
  2019-02-01  0:14 [PATCH v3] vfat: don't read garbage after last dirent Matteo Croce
@ 2019-02-13 17:29 ` OGAWA Hirofumi
  0 siblings, 0 replies; 2+ messages in thread
From: OGAWA Hirofumi @ 2019-02-13 17:29 UTC (permalink / raw)
  To: Matteo Croce; +Cc: linux-kernel, linux-fsdevel, Timothy Redaelli

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

Tested some, and this patch seems to be not working.  First
fat_readdir() exits by zero dir entry, but dir pos is updated to next
position, then second fat_readdir() continue with previous dir pos and
will read beyond zero dir entry.

So, maybe, we are better move down the zero dir entry handling into
fat_get_entry(), instead of copying several places. But like you know,
we would like to skip the zero dir entry only if read/search operation.

Maybe, adding flag to fat_get_entry() and use it, or adding small
wrapper? And in read/search case, we should not update *pos if hit the
zero dir entry.

I'm not testing the above actually though, we would need such.

Thanks.

> Reported-by: Timothy Redaelli <tredaelli@redhat.com>
> Signed-off-by: Matteo Croce <mcroce@redhat.com>
> ---
>  fs/fat/dir.c | 20 ++++++++++++--------
>  1 file changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/fs/fat/dir.c b/fs/fat/dir.c
> index 9d01db37183f..d919a1ee519c 100644
> --- a/fs/fat/dir.c
> +++ b/fs/fat/dir.c
> @@ -314,7 +314,7 @@ static int fat_parse_long(struct inode *dir, loff_t *pos,
>  
>  		if (ds->id & 0x40)
>  			(*unicode)[offset + 13] = 0;
> -		if (fat_get_entry(dir, pos, bh, de) < 0)
> +		if (fat_get_entry(dir, pos, bh, de) < 0 || !(*de)->name[0])
>  			return PARSE_EOF;
>  		if (slot == 0)
>  			break;
> @@ -476,7 +476,8 @@ int fat_search_long(struct inode *inode, const unsigned char *name,
>  
>  	err = -ENOENT;
>  	while (1) {
> -		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;
> @@ -588,7 +589,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;
> @@ -898,7 +899,8 @@ int fat_get_dotdot_entry(struct inode *dir, struct buffer_head **bh,
>  	loff_t offset = 0;
>  
>  	*de = NULL;
> -	while (fat_get_short_entry(dir, &offset, bh, de) >= 0) {
> +	while (fat_get_short_entry(dir, &offset, bh, de) >= 0 &&
> +	       (*de)->name[0]) {
>  		if (!strncmp((*de)->name, MSDOS_DOTDOT, MSDOS_NAME))
>  			return 0;
>  	}
> @@ -916,7 +918,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;
> @@ -941,7 +944,8 @@ int fat_subdirs(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 (de->attr & ATTR_DIR)
>  			count++;
>  	}
> @@ -961,7 +965,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;
> @@ -985,7 +989,7 @@ int fat_scan_logstart(struct inode *dir, int i_logstart,
>  	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 (fat_get_start(MSDOS_SB(sb), sinfo->de) == i_logstart) {
>  			sinfo->slot_off -= sizeof(*sinfo->de);
>  			sinfo->nr_slots = 1;

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

^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, back to index

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-01  0:14 [PATCH v3] vfat: don't read garbage after last dirent Matteo Croce
2019-02-13 17:29 ` OGAWA Hirofumi

Linux-Fsdevel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-fsdevel/0 linux-fsdevel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-fsdevel linux-fsdevel/ https://lore.kernel.org/linux-fsdevel \
		linux-fsdevel@vger.kernel.org linux-fsdevel@archiver.kernel.org
	public-inbox-index linux-fsdevel


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-fsdevel


AGPL code for this site: git clone https://public-inbox.org/ public-inbox