linux-kernel - Re: [PATCH v5 7/8] fat (exportfs): rebuild directory-inode if fat

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAKYAXd8SB7tz82Y57TKcngZC8vzvy1S=SAhzurA92jyobaVtNg@mail.gmail.com>
Date:	Fri, 21 Dec 2012 19:15:29 +0900
From:	Namjae Jeon <linkinjeon@...il.com>
To:	OGAWA Hirofumi <hirofumi@...l.parknet.co.jp>
Cc:	akpm@...ux-foundation.org, linux-fsdevel@...r.kernel.org,
	linux-kernel@...r.kernel.org,
	Namjae Jeon <namjae.jeon@...sung.com>,
	Ravishankar N <ravi.n1@...sung.com>,
	Amit Sahrawat <a.sahrawat@...sung.com>
Subject: Re: [PATCH v5 7/8] fat (exportfs): rebuild directory-inode if
 fat_dget() fails

2012/12/21, OGAWA Hirofumi <hirofumi@...l.parknet.co.jp>:
> Namjae Jeon <linkinjeon@...il.com> writes:
>
>>>> Hm, start with copy of fat_search_long() and refactoring it. With it,
>>>> we
>>>> will be able to avoid the fixed bugs.
>>>>
>>>> After that, we might be able to merge those somehow. Well, I'm not
>>>> pretty sure without doing it actually though.
>> Hi OGAWA.
>>
>> When we checked to merge it with fat_search_long, we really did not
>> find any possibility of code reusing for fat_traverse_cluster.
>> It will not be proper. In case of fat_search_long()-> it is performing
>> a name based lookup in a particular directory.
>> While for reconnection with parent from NFS, we do not have the name
>> of the parent directory. We are relying on ‘i_pos’ on disk position of
>> directory entry.
>> So, on first request for lookup for “..” (i.e if we call
>> fat_search_long(child_dir->d_inode, MSDOS_DOTDOT,2,slot_info) it will
>> return the directory entry for “..” itself.  From this entry we can
>> read the “log start” which is the starting cluster of the parent
>> directory, but instead we need the “directory entry” where this is
>> stored.
>> So, from this level we need to go further one level up and read the
>> parent ->parent-> cluster. After reading that cluster, we need to do a
>> lookup of the “required ipos” in this directory block.
>> i.e., if the path is A/B/C and we call the get_parent() from ‘C’. We
>> need to read the directory block contents of ‘A’ and from those
>> ‘directory entries' compare the log_start with the log_start value of
>> B, which was obtained by doing a lookup “..” in C.
>> So, Instead of it, we suggest we fix the “infinite-loop” condition in
>> fat_traverse_logic and retain the code.
>> of course, we will test it with corrupted FATfs.
>> Please share your thoughts on this.
>
> Yes, we can't use fat_search_long() as is, of course. However, we can
> share the basic algorithm and code.
>
> The both are doing,
>
> 1) traverse the blocks chained by ->i_start.
> 2) get the record (dirent) from blocks.
> 3) check the detail of record
>
> The difference is only (3), right? I know, the code has many differences
> though. The actual logic are almost same.
>
> And see, e.g., fat_get_cluster() is checking several unexpected
> state. We have to care about corrupting data. It is not only
> "infinite-loop" case.  And why I'm saying it is better to share code.

Regarding unexpected conditions,
When we compare the unexpected conditions in fat_get_cluster:
We get these as conditions which can arise:
fat_end_read() returns:
 1) < 0
 2) FAT_ENT_FREE
 3) FAT_ENT_EOF
And the last is
      4) Preventing the infinite looping of cluster chain.

Our patch is already covering the cases:
 case 1) , 2) =>  if (search_clus < 0 || search_clus == FAT_ENT_FREE)
 about  case 3) => while (search_clus != FAT_ENT_EOF);

while for Case 4:
We can make changes like this:

@@ -179,8 +179,9 @@ struct dentry *fat_get_parent(struct dentry *child_dir)
        struct inode *parent_inode = NULL;
        struct msdos_sb_info *sbi = MSDOS_SB(sb);
        int parent_logstart;
-       int search_clus, clus_to_match;
+       int search_clus, clus_to_match, clus_count = 0;
        sector_t blknr;
+       const int limit = sb->s_maxbytes >> MSDOS_SB(sb)->cluster_bits;

        if (!fat_get_dotdot_entry(child_dir->d_inode, &dotdot_bh, &de)) {
                parent_logstart = fat_get_start(sbi, de);
@@ -223,6 +224,14 @@ struct dentry *fat_get_parent(struct dentry *child_dir)
                                                search_clus, clus_to_match);
                                if (IS_ERR(parent_inode) || parent_inode)
                                        break;
+                               if (++clus_count > limit) {
+                                       fat_fs_error_ratelimit(sb,
+                                       "%s: detected the cluster chain loop"
+                                       " while reading directory entries from"
+                                       " cluster %d", __func__, search_clus);
+                                       parent_inode = ERR_PTR(-EIO);
+                                       break;
+                               }
                                fatent_init(&fatent);
                                search_clus = fat_ent_read(sb, &fatent,
                                                           search_clus);
-- 
1.7.7.6

Are we missing anything more in this?

Thanks.
>
> Thanks.
> --
> OGAWA Hirofumi <hirofumi@...l.parknet.co.jp>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/