[PATCH] isofs: fix High Sierra dirent flag accesses

[PATCH] isofs: fix High Sierra dirent flag accesses

[Egor Chelak]

The flags byte of the dirent was accessed as de->flags[0] in a couple of
places, and not as de->flags[-sbi->s_high_sierra], which is how it's
accessed elsewhere. This caused a bug, where some files on an HSF disc
could be inaccessible.

For context, here is the difference between HSF dirents and ISO dirents:
Offset  | High Sierra | ISO-9660       | struct iso_directory_record
Byte 24 | Flags       | mtime timezone | de->date[6] (de->flags[-1])
Byte 25 | Reserved    | Flags          | de->flags[0]

In a particular HSF disc image that I have, the reserved byte is
arbitrary. Some regular files ended up having the directory bit (0x02)
set in the reserved byte. isofs_normalize_block_and_offset would
interpret that byte as the flags byte, and try to normalize the dirent
as if it was pointing to a directory. Then, when the file is looked up,
its inode gets filled with garbage data (file contents interpreted as
directory entry), making it unreadable.

Signed-off-by: Egor Chelak <egor.chelak@gmail.com>
---
 fs/isofs/dir.c    | 6 ++++--
 fs/isofs/export.c | 6 +++++-
 fs/isofs/isofs.h  | 5 +++--
 fs/isofs/namei.c  | 3 ++-
 4 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/fs/isofs/dir.c b/fs/isofs/dir.c
index f0fe641893a5..5171dbbcda81 100644
--- a/fs/isofs/dir.c
+++ b/fs/isofs/dir.c
@@ -50,6 +50,7 @@ int isofs_name_translate(struct iso_directory_record *de, char *new, struct inod
 int get_acorn_filename(struct iso_directory_record *de,
 			    char *retname, struct inode *inode)
 {
+	struct isofs_sb_info *sbi = ISOFS_SB(inode->i_sb);
 	int std;
 	unsigned char *chr;
 	int retnamlen = isofs_name_translate(de, retname, inode);
@@ -66,7 +67,7 @@ int get_acorn_filename(struct iso_directory_record *de,
 		return retnamlen;
 	if ((*retname == '_') && ((chr[19] & 1) == 1))
 		*retname = '!';
-	if (((de->flags[0] & 2) == 0) && (chr[13] == 0xff)
+	if (((de->flags[-sbi->s_high_sierra] & 2) == 0) && (chr[13] == 0xff)
 		&& ((chr[12] & 0xf0) == 0xf0)) {
 		retname[retnamlen] = ',';
 		sprintf(retname+retnamlen+1, "%3.3x",
@@ -158,7 +159,8 @@ static int do_isofs_readdir(struct inode *inode, struct file *file,
 		if (first_de) {
 			isofs_normalize_block_and_offset(de,
 							&block_saved,
-							&offset_saved);
+							&offset_saved,
+							sbi->s_high_sierra);
 			inode_number = isofs_get_ino(block_saved,
 							offset_saved, bufbits);
 		}
diff --git a/fs/isofs/export.c b/fs/isofs/export.c
index 35768a63fb1d..8a8aa442ab82 100644
--- a/fs/isofs/export.c
+++ b/fs/isofs/export.c
@@ -50,6 +50,7 @@ static struct dentry *isofs_export_get_parent(struct dentry *child)
 	struct iso_directory_record *de = NULL;
 	struct buffer_head * bh = NULL;
 	struct dentry *rv = NULL;
+	struct isofs_sb_info *sbi = ISOFS_SB(child_inode->i_sb);
 
 	/* "child" must always be a directory. */
 	if (!S_ISDIR(child_inode->i_mode)) {
@@ -97,7 +98,10 @@ static struct dentry *isofs_export_get_parent(struct dentry *child)
 	}
 
 	/* Normalize */
-	isofs_normalize_block_and_offset(de, &parent_block, &parent_offset);
+	isofs_normalize_block_and_offset(de,
+					 &parent_block,
+					 &parent_offset,
+					 sbi->s_high_sierra);
 
 	rv = d_obtain_alias(isofs_iget(child_inode->i_sb, parent_block,
 				     parent_offset));
diff --git a/fs/isofs/isofs.h b/fs/isofs/isofs.h
index 055ec6c586f7..5c3b8f065a9a 100644
--- a/fs/isofs/isofs.h
+++ b/fs/isofs/isofs.h
@@ -186,10 +186,11 @@ static inline unsigned long isofs_get_ino(unsigned long block,
 static inline void
 isofs_normalize_block_and_offset(struct iso_directory_record* de,
 				 unsigned long *block,
-				 unsigned long *offset)
+				 unsigned long *offset,
+				 int high_sierra)
 {
 	/* Only directories are normalized. */
-	if (de->flags[0] & 2) {
+	if (de->flags[-high_sierra] & 2) {
 		*offset = 0;
 		*block = (unsigned long)isonum_733(de->extent)
 			+ (unsigned long)isonum_711(de->ext_attr_length);
diff --git a/fs/isofs/namei.c b/fs/isofs/namei.c
index cac468f04820..d732964755f4 100644
--- a/fs/isofs/namei.c
+++ b/fs/isofs/namei.c
@@ -138,7 +138,8 @@ isofs_find_entry(struct inode *dir, struct dentry *dentry,
 		if (match) {
 			isofs_normalize_block_and_offset(de,
 							 &block_saved,
-							 &offset_saved);
+							 &offset_saved,
+							 sbi->s_high_sierra);
 			*block_rv = block_saved;
 			*offset_rv = offset_saved;
 			brelse(bh);
-- 
2.27.0

Re: [PATCH] isofs: fix High Sierra dirent flag accesses

[Matthew Wilcox]

On Sun, Jun 21, 2020 at 07:08:17AM +0300, Egor Chelak wrote:
> The flags byte of the dirent was accessed as de->flags[0] in a couple of
> places, and not as de->flags[-sbi->s_high_sierra], which is how it's
> accessed elsewhere. This caused a bug, where some files on an HSF disc
> could be inaccessible.

> +++ b/fs/isofs/dir.c
> @@ -50,6 +50,7 @@ int isofs_name_translate(struct iso_directory_record *de, char *new, struct inod
>  int get_acorn_filename(struct iso_directory_record *de,
>  			    char *retname, struct inode *inode)
>  {
> +	struct isofs_sb_info *sbi = ISOFS_SB(inode->i_sb);
>  	int std;
>  	unsigned char *chr;
>  	int retnamlen = isofs_name_translate(de, retname, inode);
> @@ -66,7 +67,7 @@ int get_acorn_filename(struct iso_directory_record *de,
>  		return retnamlen;
>  	if ((*retname == '_') && ((chr[19] & 1) == 1))
>  		*retname = '!';
> -	if (((de->flags[0] & 2) == 0) && (chr[13] == 0xff)
> +	if (((de->flags[-sbi->s_high_sierra] & 2) == 0) && (chr[13] == 0xff)
>  		&& ((chr[12] & 0xf0) == 0xf0)) {
>  		retname[retnamlen] = ',';
>  		sprintf(retname+retnamlen+1, "%3.3x",

It's been about 22 years since I contributed the patch which added
support for the Acorn extensions ;-)  But I'm pretty sure that it's not
possible to have an Acorn CD-ROM that is also an HSF CD-ROM.  That is,
all Acorn formatted CD-ROMs are ISO-9660 compatible.  So I think this
chunk of the patch is not required.

Re: [PATCH] isofs: fix High Sierra dirent flag accesses

[Matthew Wilcox]

On Sun, Jun 21, 2020 at 07:08:17AM +0300, Egor Chelak wrote:
> The flags byte of the dirent was accessed as de->flags[0] in a couple of
> places, and not as de->flags[-sbi->s_high_sierra], which is how it's
> accessed elsewhere. This caused a bug, where some files on an HSF disc
> could be inaccessible.
> 
> For context, here is the difference between HSF dirents and ISO dirents:
> Offset  | High Sierra | ISO-9660       | struct iso_directory_record
> Byte 24 | Flags       | mtime timezone | de->date[6] (de->flags[-1])
> Byte 25 | Reserved    | Flags          | de->flags[0]

Also, ew.  Why on earth do we do 'de->flags[-sbi->s_high_sierra]'?
I'm surprised we don't have any tools that warn about references outside
an array.  I would do this as ...

static inline u8 de_flags(struct isofs_sb_info *sbi,
		struct iso_directory_record *de)
{
	if (sbi->s_high_sierra)
		return de->date[6];
	return de->flags;
}

Re: [PATCH] isofs: fix High Sierra dirent flag accesses

[Egor Chelak]

On 6/23/2020 12:22 AM, Matthew Wilcox wrote:
> It's been about 22 years since I contributed the patch which added
> support for the Acorn extensions ;-)  But I'm pretty sure that it's not
> possible to have an Acorn CD-ROM that is also an HSF CD-ROM.  That is,
> all Acorn formatted CD-ROMs are ISO-9660 compatible.  So I think this
> chunk of the patch is not required.

I couldn't find any info on Acorn extensions online, so I wasn't sure if
they were mutually exclusive or not, and fixed it there too, just to be
safe. Still, even though it won't be needed in practice, I think it's
better to access the flags in the same way everywhere. Having the same
field accessed differently in different places raises the question "why
it's done differently here?". If we go that way, at the very least there
should be an explanatory comment saying HSF+Acorn is an impossible
combination, and perhaps some logic to prevent HSF discs from mounting
with -o map=acorn. Just leaving it be doesn't seem like a clean
solution.

On 6/23/2020 12:31 AM, Matthew Wilcox wrote:
> Also, ew.  Why on earth do we do 'de->flags[-sbi->s_high_sierra]'?
> I'm surprised we don't have any tools that warn about references outside
> an array.  I would do this as ...
> 
> static inline u8 de_flags(struct isofs_sb_info *sbi,
> 		struct iso_directory_record *de)
> {
> 	if (sbi->s_high_sierra)
> 		return de->date[6];
> 	return de->flags;
> }
I would do something like that, but for this patch I'm just trying to do
a simple bugfix. The isofs code definitely needs a clean up, and perhaps
I'll do it in a future patch. I haven't submitted a patch before, so I
want to start with something simple and uncontroversial, while I learn
the process. :-)