| Message ID | d482bb9d-19d2-c4e9-08a0-1c0b9d58776c@snapit.group |
|---|---|
| State | Accepted |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | fs/squashfs: Fix some hardlinks reading the wrong inode | expand |
Hello, Campbell Suter <campbell@snapit.group> wrote on Fri, 30 Apr 2021 16:45:46 +1200: > In SquashFS, the contents of a directory is stored by > squashfs_directory_entry structures which contain the file's name, inode > and position within the filesystem. > > The inode number is not stored directly; instead each directory has one > or more headers which set a base inode number, and files store the > offset from that to the file's inode number. > > In mksquashfs, each inode is allocated a number in the same order as > they are written to the directory table; thus the offset from the > header's base inode number to the file's inode number is usually > positive. > > Hardlinks are simply stored with two directory entries referencing the > same file. This means the second entry will thus have an inode number > much lower than the surrounding files. Since the header's base inode > number comes from the first entry that uses the header, this delta will > usually be negative. I am not sure hardlinks have been tested extensively (even tested at all, actually?) so the logic here looks good to me. > Previously, U-Boot's squashfs_directory_entry.inode_offset field was > declared as an unsigned value. Thus when a negative value was found, it > would either resolve to an invalid inode number or to that of an > unrelated file. That's also what the documentation used to write this code actually states: https://dr-emann.github.io/squashfs/ # "Directory Entry" section: Name Type Description offset u16 An offset into the uncompressed inode metadata block inode offset i16 The difference of this inode's number to the reference stored in the header type u16 The inode type [...] name_size u16 One less than the size of the entry name name u8[name_size + 1] The file name of the entry without a trailing null byte And I believe i16 means int16_t which is a signed value indeed. > A squashfs image to test this can be created like so: > > echo hi > sqfs_test_files/001-root-file > mkdir sqfs_test_files/002-subdir > touch sqfs_test_files/002-subdir/003-file > ln sqfs_test_files/{001-root-file,002-subdir/004-link} > mksquashfs sqfs_test_files/ test.sqfs -noappend > > Note that squashfs sorts the files ASCIIbetacally, so we can use the > names to control the order they appear in. The ordering is important - > the first reference to the file must have a lower inode number than the > directory in which the second reference resides, and the second > reference cannot be the first file in the directory. > > Listing this sample image in U-Boot results in: > > => sqfsls virtio 2 002-subdir > 0 003-file > Inode not found. > 0 004-link > > Signed-off-by: Campbell Suter <campbell@snapit.group> > --- > > fs/squashfs/sqfs_filesystem.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/squashfs/sqfs_filesystem.h b/fs/squashfs/sqfs_filesystem.h > index 856cd15e349..5440b6c0e05 100644 > --- a/fs/squashfs/sqfs_filesystem.h > +++ b/fs/squashfs/sqfs_filesystem.h > @@ -231,7 +231,7 @@ union squashfs_inode { > > struct squashfs_directory_entry { > u16 offset; > - u16 inode_offset; > + s16 inode_offset; > u16 type; > u16 name_size; > char name[0]; Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com> Thanks, Miquèl
On Fri, Apr 30, 2021 at 04:45:46PM +1200, Campbell Suter wrote: > In SquashFS, the contents of a directory is stored by > squashfs_directory_entry structures which contain the file's name, inode > and position within the filesystem. > > The inode number is not stored directly; instead each directory has one > or more headers which set a base inode number, and files store the > offset from that to the file's inode number. > > In mksquashfs, each inode is allocated a number in the same order as > they are written to the directory table; thus the offset from the > header's base inode number to the file's inode number is usually > positive. > > Hardlinks are simply stored with two directory entries referencing the > same file. This means the second entry will thus have an inode number > much lower than the surrounding files. Since the header's base inode > number comes from the first entry that uses the header, this delta will > usually be negative. > > Previously, U-Boot's squashfs_directory_entry.inode_offset field was > declared as an unsigned value. Thus when a negative value was found, it > would either resolve to an invalid inode number or to that of an > unrelated file. > > A squashfs image to test this can be created like so: > > echo hi > sqfs_test_files/001-root-file > mkdir sqfs_test_files/002-subdir > touch sqfs_test_files/002-subdir/003-file > ln sqfs_test_files/{001-root-file,002-subdir/004-link} > mksquashfs sqfs_test_files/ test.sqfs -noappend > > Note that squashfs sorts the files ASCIIbetacally, so we can use the > names to control the order they appear in. The ordering is important - > the first reference to the file must have a lower inode number than the > directory in which the second reference resides, and the second > reference cannot be the first file in the directory. > > Listing this sample image in U-Boot results in: > > => sqfsls virtio 2 002-subdir > 0 003-file > Inode not found. > 0 004-link > > Signed-off-by: Campbell Suter <campbell@snapit.group> > Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com> Applied to u-boot/master, thanks!
diff --git a/fs/squashfs/sqfs_filesystem.h b/fs/squashfs/sqfs_filesystem.h index 856cd15e349..5440b6c0e05 100644 --- a/fs/squashfs/sqfs_filesystem.h +++ b/fs/squashfs/sqfs_filesystem.h @@ -231,7 +231,7 @@ union squashfs_inode { struct squashfs_directory_entry { u16 offset; - u16 inode_offset; + s16 inode_offset; u16 type; u16 name_size; char name[0];
In SquashFS, the contents of a directory is stored by squashfs_directory_entry structures which contain the file's name, inode and position within the filesystem. The inode number is not stored directly; instead each directory has one or more headers which set a base inode number, and files store the offset from that to the file's inode number. In mksquashfs, each inode is allocated a number in the same order as they are written to the directory table; thus the offset from the header's base inode number to the file's inode number is usually positive. Hardlinks are simply stored with two directory entries referencing the same file. This means the second entry will thus have an inode number much lower than the surrounding files. Since the header's base inode number comes from the first entry that uses the header, this delta will usually be negative. Previously, U-Boot's squashfs_directory_entry.inode_offset field was declared as an unsigned value. Thus when a negative value was found, it would either resolve to an invalid inode number or to that of an unrelated file. A squashfs image to test this can be created like so: echo hi > sqfs_test_files/001-root-file mkdir sqfs_test_files/002-subdir touch sqfs_test_files/002-subdir/003-file ln sqfs_test_files/{001-root-file,002-subdir/004-link} mksquashfs sqfs_test_files/ test.sqfs -noappend Note that squashfs sorts the files ASCIIbetacally, so we can use the names to control the order they appear in. The ordering is important - the first reference to the file must have a lower inode number than the directory in which the second reference resides, and the second reference cannot be the first file in the directory. Listing this sample image in U-Boot results in: => sqfsls virtio 2 002-subdir 0 003-file Inode not found. 0 004-link Signed-off-by: Campbell Suter <campbell@snapit.group> --- fs/squashfs/sqfs_filesystem.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)