NetBSD Problem Report #52993

From www@NetBSD.org  Thu Feb  8 11:09:43 2018
Return-Path: <www@NetBSD.org>
Received: from mail.netbsd.org (mail.netbsd.org [199.233.217.200])
	(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
	(Client CN "mail.NetBSD.org", Issuer "mail.NetBSD.org CA" (not verified))
	by mollari.NetBSD.org (Postfix) with ESMTPS id 1E5927A18A
	for <gnats-bugs@gnats.NetBSD.org>; Thu,  8 Feb 2018 11:09:43 +0000 (UTC)
Message-Id: <20180208110941.C38697A231@mollari.NetBSD.org>
Date: Thu,  8 Feb 2018 11:09:41 +0000 (UTC)
From: "Harold Gutch" <logix@foobar.franken.de>
Reply-To: "Harold Gutch" <logix@foobar.franken.de>
To: gnats-bugs@NetBSD.org
Subject: case sensitive HFS+ can return data from wrong file
X-Send-Pr-Version: www-1.0

>Number:         52993
>Category:       kern
>Synopsis:       case sensitive HFS+ can return data from wrong file
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    kern-bug-people
>State:          needs-pullups
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Thu Feb 08 11:10:00 +0000 2018
>Closed-Date:    
>Last-Modified:  Mon Jul 13 20:12:38 +0000 2020
>Originator:     Harold Gutch
>Release:        NetBSD 7.1 (but the bug exists from 5.0 to HEAD)
>Organization:
>Environment:
NetBSD  7.1 NetBSD 7.1 (GENERIC.201703111743Z) amd64
>Description:
On a case sensitive HFS+ file system, when locating a file of a given name, file names are compared against it as memcmp(name1, name2, min(len(name1), len(name2))).  Thus, if name1 starts with name2 (or vice versa) they are understood to be identical and as a result when accessing name1 we might end up reading from name2 instead.

The issue does not occur for case insensitive HFS+ file systems (the default).
>How-To-Repeat:
1) On Mac OS, create a case sensitive HFS+ file system, create two files one's name starts with the other's, and fill them with different data.

osx:tmp hg$ hdiutil create test.dmg -size 1m -fs HFS+X -volname "test"
...............................................................................
created: /Users/hg/tmp/test.dmg
osx:tmp hg$ hdiutil mount test.dmg
/dev/disk7              GUID_partition_scheme
/dev/disk7s1            Apple_HFS                       /Volumes/test
osx:tmp hg$ echo foo > /Volumes/test/test
osx:tmp hg$ echo bar > /Volumes/test/test123
osx:tmp hg$ cat /Volumes/test/test
foo
osx:tmp hg$ cat /Volumes/test/test123
bar


2) After umounting, copy the .dmg over to NetBSD.  There mount and check the inode numbers and the contents of the files.

ichiban# vndconfig vnd0 test.dmg
ichiban# gpt show vnd0 | grep HFS
     40   1968      1  GPT part - Apple HFS
ichiban# dkctl vnd0 addwedge test 40 1968 hfs
dk0 created successfully.
ichiban# mount -t hfs -o ro /dev/dk0 /mnt
ichiban# cat /mnt/test
foo
ichiban# cat /mnt/test123
foo
ichiban# ls -li /mnt/test123 /mnt/test
22 -rw-r--r--  1 503  staff  4 Feb  6 14:47 /mnt/test
22 -rw-r--r--  1 503  staff  4 Feb  6 14:47 /mnt/test123


3) Notice that on NetBSD both, the contents of test and test123, and also their inode number are identical.
>Fix:
[ NOTE: the actual fix is in the few lines after "/* FIXME: ...", the rest is merely a bit of syntax sugar without functional change that just avoids the repeated casts to (const hfs_catalog_key_t*) all over the function and the same then again in hfslib_compare_extent_keys(). ]


--- src/sys/fs/hfs/libhfs.c.orig	2015-06-21 13:40:25.000000000 +0000
+++ src/sys/fs/hfs/libhfs.c	2018-02-06 11:51:04.000000000 +0000
@@ -2384,37 +2384,42 @@
 /* binary compare (i.e., not case folding) */
 int
 hfslib_compare_catalog_keys_bc (
-	const void *a,
-	const void *b)
+	const void *ap,
+	const void *bp)
 {
-	if (((const hfs_catalog_key_t*)a)->parent_cnid
-		== ((const hfs_catalog_key_t*)b)->parent_cnid)
+	int c;
+	const hfs_catalog_key_t *a, *b;
+
+	a = (const hfs_catalog_key_t *) ap;
+	b = (const hfs_catalog_key_t *) bp;
+
+	if (a->parent_cnid == b->parent_cnid)
 	{
-		if (((const hfs_catalog_key_t*)a)->name.length == 0 &&
-			((const hfs_catalog_key_t*)b)->name.length == 0)
+		if (a->name.length == 0 && b->name.length == 0)
 			return 0;

-		if (((const hfs_catalog_key_t*)a)->name.length == 0)
+		if (a->name.length == 0)
 			return -1;
-		if (((const hfs_catalog_key_t*)b)->name.length == 0)
+		if (b->name.length == 0)
 			return 1;

 		/* FIXME: This does a byte-per-byte comparison, whereas the HFS spec
 		 * mandates a uint16_t chunk comparison. */
-		return memcmp(((const hfs_catalog_key_t*)a)->name.unicode,
-			((const hfs_catalog_key_t*)b)->name.unicode,
-			min(((const hfs_catalog_key_t*)a)->name.length,
-				((const hfs_catalog_key_t*)b)->name.length));
+		c = memcmp(a->name.unicode, b->name.unicode,
+			sizeof(unichar_t)*min(a->name.length, b->name.length));
+		if (c != 0)
+			return c;
+		else
+			return (a->name.length - b->name.length);
 	} else {
-		return (((const hfs_catalog_key_t*)a)->parent_cnid - 
-				((const hfs_catalog_key_t*)b)->parent_cnid);
+		return (a->parent_cnid - b->parent_cnid);
 	}
 }

 int
 hfslib_compare_extent_keys (
-	const void *a,
-	const void *b)
+	const void *ap,
+	const void *bp)
 {
 	/*
 	 *	Comparison order, in descending importance:
@@ -2422,27 +2427,25 @@
 	 *		CNID -> fork type -> start block
 	 */

-	if (((const hfs_extent_key_t*)a)->file_cnid
-		== ((const hfs_extent_key_t*)b)->file_cnid)
+	const hfs_extent_key_t *a, *b;
+	a = (const hfs_extent_key_t *) ap;
+	b = (const hfs_extent_key_t *) bp;
+
+	if (a->file_cnid == b->file_cnid)
 	{
-		if (((const hfs_extent_key_t*)a)->fork_type
-			== ((const hfs_extent_key_t*)b)->fork_type)
+		if (a->fork_type == b->fork_type)
 		{
-			if (((const hfs_extent_key_t*)a)->start_block
-				== ((const hfs_extent_key_t*)b)->start_block)
+			if (a->start_block == b->start_block)
 			{
 				return 0;
 			} else {
-				return (((const hfs_extent_key_t*)a)->start_block - 
-						((const hfs_extent_key_t*)b)->start_block);
+				return (a->start_block - b->start_block);
 			}
 		} else {
-			return (((const hfs_extent_key_t*)a)->fork_type - 
-					((const hfs_extent_key_t*)b)->fork_type);
+			return (a->fork_type - b->fork_type);
 		}
 	} else {
-		return (((const hfs_extent_key_t*)a)->file_cnid - 
-				((const hfs_extent_key_t*)b)->file_cnid);
+		return (a->file_cnid - b->file_cnid);
 	}
 }


>Release-Note:

>Audit-Trail:
From: Sevan Janiyan <sevan@NetBSD.org>
To: gnats-bugs@NetBSD.org
Cc: 
Subject: Re: kern/52993: case sensitive HFS+ can return data from wrong file
Date: Fri, 9 Feb 2018 16:35:20 +0000

 I've uploaded a test disk image created as instructed in step #1

 https://www.netbsd.org/~sevan/kern52993-test.dmg


 Sevan

Responsible-Changed-From-To: kern-bug-people->sevan
Responsible-Changed-By: sevan@NetBSD.org
Responsible-Changed-When: Sat, 17 Feb 2018 00:44:07 +0000
Responsible-Changed-Why:
take


From: Sevan Janiyan <sevan@NetBSD.org>
To: gnats-bugs@NetBSD.org
Cc: 
Subject: Re: kern/52993 (case sensitive HFS+ can return data from wrong file)
Date: Fri, 28 Dec 2018 04:12:03 +0000

 With the supplied patch the issue is still not resolved.


 Sevan

From: Harold Gutch <logix@foobar.franken.de>
To: gnats-bugs@NetBSD.org
Cc: 
Subject: Re: kern/52993 (case sensitive HFS+ can return data from wrong file)
Date: Sun, 30 Dec 2018 17:08:19 +0100

 On Fri, Dec 28, 2018 at 04:15:00AM +0000, Sevan Janiyan wrote:
 >  With the supplied patch the issue is still not resolved.

 It works for me.  I have been using it all this time on a Raspberry Pi 1
 (albeit only under light load) with multiple case sensitive HFS+ file
 systems.

 This here is from a vanilla 8.0 amd64 installation in VirtualBox,
 with your disk image:

 localhost# mv /stand/amd64/8.0/modules/hfs/hfs.kmod /stand/amd64/8.0/modules/hfs/hfs_orig.kmod
 localhost# mv /stand/amd64/8.0/modules/hfs /stand/amd64/8.0/modules/hfs_orig
 localhost# cd /usr/src/sys/fs/hfs
 localhost# patch -p4 < patch
 localhost# cd /usr/src
 localhost# ./build.sh modules
 localhost# mv /usr/obj/destdir.amd64/stand/amd64/8.0/modules/hfs /stand/amd64/8.0/modules/

 localhost# vndconfig vnd0 kern52993-test.dmg
 localhost# modload hfs_orig
 localhost# mount -t hfs -o ro /dev/dk0 /mnt
 localhost# ls -li /mnt/test /mnt/test123
 23 -rw-r--r--  1 501  staff  4 Feb  9  2018 /mnt/test
 23 -rw-r--r--  1 501  staff  4 Feb  9  2018 /mnt/test123
 localhost# cat /mnt/test /mnt/test123
 foo
 foo
 localhost# umount /mnt
 localhost# modunload hfs
   [ although the module's file name/directory is "hfs_orig",
     the module's (internal) name still is "hfs" ]

 localhost# modload hfs
 localhost# mount -t hfs -o ro /dev/dk0 /mnt
 localhost# ls -li /mnt/test /mnt/test123
 23 -rw-r--r--  1 501  staff  4 Feb  9  2018 /mnt/test
 24 -rw-r--r--  1 501  staff  4 Feb  9  2018 /mnt/test123
 localhost# cat /mnt/test /mnt/test123
 foo
 bar
 localhost#

 If you want I can redo this test with current, but as sys/fs/hfs is
 virtually unchanged this shouldn't change anything.  I am not sure
 what else I can add right now.

 Or do you have some other issue with the patched module?


   Harold

From: Sevan Janiyan <sevan@NetBSD.org>
To: gnats-bugs@NetBSD.org
Cc: 
Subject: Re: kern/52993 (case sensitive HFS+ can return data from wrong file)
Date: Sun, 30 Dec 2018 22:10:24 +0000

 Hi Harold,

 On 30/12/2018 16:10, Harold Gutch wrote:
 >  If you want I can redo this test with current, but as sys/fs/hfs is
 >  virtually unchanged this shouldn't change anything.  I am not sure
 >  what else I can add right now.
 >  
 >  Or do you have some other issue with the patched module?

 Apologies, I made a mistake in that I hadn't realised HFS support was
 via a loadable module, hence I only built a new kernel. I have just
 built a new set of modules & can see the issue is now resolved.

 optislab# vnconfig vnd0 /home/me/kern52993-test.dmg
 optislab# mount_hfs /dev/dk0 /mnt


 optislab# cat /mnt/test


 foo
 optislab# cat /mnt/test123


 bar
 optislab# ls -li /mnt/
 total 42
 20 -rw-r--r--  1 501   staff  8196 Feb  9  2018 .DS_Store
 17 dr-xr-xr-t  1 root  wheel   512 Feb  9  2018 .HFS+ Private Directory
 Data?
 18 drwx------  1 501   staff   512 Feb  9  2018 .fseventsd
 23 -rw-r--r--  1 501   staff     4 Feb  9  2018 test
 24 -rw-r--r--  1 501   staff     4 Feb  9  2018 test123


 Sevan

From: "Sevan Janiyan" <sevan@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/52993 CVS commit: src/sys/fs/hfs
Date: Sun, 30 Dec 2018 22:40:00 +0000

 Module Name:	src
 Committed By:	sevan
 Date:		Sun Dec 30 22:40:00 UTC 2018

 Modified Files:
 	src/sys/fs/hfs: libhfs.c

 Log Message:
 Fix support for case sensitive HFS.
 Without this change, the wrong file is returned, if 2 file names contain a
 subset of each other.

 Code submitted in PR bin/52993 by Harold Gutch


 To generate a diff of this commit:
 cvs rdiff -u -r1.14 -r1.15 src/sys/fs/hfs/libhfs.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

State-Changed-From-To: open->needs-pullups
State-Changed-By: sevan@NetBSD.org
State-Changed-When: Sun, 30 Dec 2018 23:02:19 +0000
State-Changed-Why:
Need an ATF test too.
(in progress)


From: Harold Gutch <logix@foobar.franken.de>
To: gnats-bugs@NetBSD.org
Cc: 
Subject: Re: kern/52993 (case sensitive HFS+ can return data from wrong file)
Date: Mon, 31 Dec 2018 00:04:56 +0100

 Hi Sevan,

 On Sun, Dec 30, 2018 at 10:15:00PM +0000, Sevan Janiyan wrote:
 >  Apologies, I made a mistake in that I hadn't realised HFS support was
 >  via a loadable module, hence I only built a new kernel. I have just
 >  built a new set of modules & can see the issue is now resolved.

 Ah, OK.  I think I only tried this with hfs as a module, but not
 statically in-kernel or via puffs.  Not that it *should* make a
 difference of course.


 thanks,
   Harold

Responsible-Changed-From-To: sevan->kern-bug-people
Responsible-Changed-By: wiz@NetBSD.org
Responsible-Changed-When: Mon, 13 Jul 2020 20:12:38 +0000
Responsible-Changed-Why:
Reset to role account


>Unformatted:

NetBSD Home
NetBSD PR Database Search

(Contact us) $NetBSD: query-full-pr,v 1.46 2020/01/03 16:35:01 leot Exp $
$NetBSD: gnats_config.sh,v 1.9 2014/08/02 14:16:04 spz Exp $
Copyright © 1994-2020 The NetBSD Foundation, Inc. ALL RIGHTS RESERVED.