Hi, On Wed, Aug 12, 2020 at 11:59:04AM +0300, Dan Carpenter wrote: > Yeah, the patch doesn't work at all. I looked at one call tree and it > is: > > hfs_mdb_get() tries to allocate HFS_SB(sb)->ext_tree. > > HFS_SB(sb)->ext_tree = hfs_btree_open(sb, HFS_EXT_CNID, hfs_ext_keycmp); > ^^^^^^^^ > > hfs_btree_open() calls page = read_mapping_page(mapping, 0, NULL); > read_mapping_page() calls mapping->a_ops->readpage() which leads to > hfs_readpage() which leads to hfs_ext_read_extent() which calls > res = hfs_find_init(HFS_SB(inode->i_sb)->ext_tree, &fd); > ^^^^^^^^ > > So we need ->ext_tree to be non-NULL before we can set ->ext_tree to be > non-NULL... :/ For HFS+, the first 8 extents for a file are kept inside its own fork data structure, not in the extent tree. So, in normal operation, you don't need to search the extent tree to find the first page of the extent tree itself. The HFS layout is different, but it should work the same way. Of course this sort of thing can still be triggered by crafted filesystems. If that's what the reproducer is about, I think just returning an error is reasonable. But these modules will never be safe against attacks such as this. > I wonder how long this has been broken and if we should just delete the > AFS file system. > > regards, > dan carpenter
