hfsplus: Fix bug causing custom uid and gid being unable to be assigned with mount

  From: Aditya Garg <gargaditya08@live.com>

Inspite of specifying UID and GID in mount command, the specified UID and
GID was not being assigned. This patch fixes this issue.

Signed-off-by: Aditya Garg <gargaditya08@live.com>
---
 fs/hfsplus/hfsplus_fs.h | 2 ++
 fs/hfsplus/inode.c      | 4 ++--
 fs/hfsplus/options.c    | 2 ++
 3 files changed, 6 insertions(+), 2 deletions(-)
  

> On Dec 2, 2022, at 7:39 AM, Aditya Garg <gargaditya08@live.com> wrote:
> 
> From: Aditya Garg <gargaditya08@live.com>
> 
> Inspite of specifying UID and GID in mount command, the specified UID and
> GID was not being assigned. This patch fixes this issue.
> 
> Signed-off-by: Aditya Garg <gargaditya08@live.com>
> ---
> fs/hfsplus/hfsplus_fs.h | 2 ++
> fs/hfsplus/inode.c      | 4 ++--
> fs/hfsplus/options.c    | 2 ++
> 3 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/hfsplus/hfsplus_fs.h b/fs/hfsplus/hfsplus_fs.h
> index a5db2e3b2..6aa919e59 100644
> --- a/fs/hfsplus/hfsplus_fs.h
> +++ b/fs/hfsplus/hfsplus_fs.h
> @@ -198,6 +198,8 @@ struct hfsplus_sb_info {
> #define HFSPLUS_SB_HFSX		3
> #define HFSPLUS_SB_CASEFOLD	4
> #define HFSPLUS_SB_NOBARRIER	5
> +#define HFSPLUS_SB_UID		6
> +#define HFSPLUS_SB_GID		7
> 
> static inline struct hfsplus_sb_info *HFSPLUS_SB(struct super_block *sb)
> {
> diff --git a/fs/hfsplus/inode.c b/fs/hfsplus/inode.c
> index aeab83ed1..4d1077db8 100644
> --- a/fs/hfsplus/inode.c
> +++ b/fs/hfsplus/inode.c
> @@ -192,11 +192,11 @@ static void hfsplus_get_perms(struct inode *inode,
> 	mode = be16_to_cpu(perms->mode);
> 
> 	i_uid_write(inode, be32_to_cpu(perms->owner));
> -	if (!i_uid_read(inode) && !mode)
> +	if (test_bit(HFSPLUS_SB_UID, &sbi->flags))
> 		inode->i_uid = sbi->uid;
> 
> 	i_gid_write(inode, be32_to_cpu(perms->group));
> -	if (!i_gid_read(inode) && !mode)
> +	if (test_bit(HFSPLUS_SB_GID, &sbi->flags))
> 		inode->i_gid = sbi->gid;

I am slightly confused. Do you mean that all files/folders will have the same UID/GID always?
What if user changes the UID/GID a particular file/folder? Also, what if we mounted
file system without specifying the UID/GID, then what UID/GID will be returned by
your logic?

Thanks,
Slava.

> 
> 	if (dir) {
> diff --git a/fs/hfsplus/options.c b/fs/hfsplus/options.c
> index 047e05c57..10a0bdacb 100644
> --- a/fs/hfsplus/options.c
> +++ b/fs/hfsplus/options.c
> @@ -137,6 +137,7 @@ int hfsplus_parse_options(char *input, struct hfsplus_sb_info *sbi)
> 				return 0;
> 			}
> 			sbi->uid = make_kuid(current_user_ns(), (uid_t)tmp);
> +			set_bit(HFSPLUS_SB_UID, &sbi->flags);
> 			if (!uid_valid(sbi->uid)) {
> 				pr_err("invalid uid specified\n");
> 				return 0;
> @@ -148,6 +149,7 @@ int hfsplus_parse_options(char *input, struct hfsplus_sb_info *sbi)
> 				return 0;
> 			}
> 			sbi->gid = make_kgid(current_user_ns(), (gid_t)tmp);
> +			set_bit(HFSPLUS_SB_GID, &sbi->flags);
> 			if (!gid_valid(sbi->gid)) {
> 				pr_err("invalid gid specified\n");
> 				return 0;
> -- 
> 2.38.1
>
  

> I am slightly confused. Do you mean that all files/folders will have the same UID/GID always?

Description about this mount option is given in Documentation/filesystems/hfsplus.rst

The UIDs and GIDs of files written by macOS 13 on any HFS+ volume are 99 by default and for iPadOS 16, it is 501.
So, writing to/editing these files on Linux causes File permission errors.

The UID/GID options in mount kinda spoofs the UIDs/GIDs of all the files in a volume to the ones specified by the user.

So for example if I run "sudo mount -o uid=1000,gid=1000 /dev/sda1 /mnt”, where /dev/sda1 is my HFS+ volume, it will be mounted at /mnt and all the files and folders over there will have uid as well as gid as 1000, which is generally the username in most Linux distributions, thus making me the owner of the files.

> What if user changes the UID/GID a particular file/folder?

Note that, the above mount option is just a spoofing and actually doesn't change the permission of the files. So the files written by macOS shall still have 99 as their UID/GID and those written by iPadOS have 501.

If you want to actually change the user/group of a file/folder, use chown. The change will be persistent. Although, if partition is remounted with UID/GID options, again the driver will spoof the ownership, but the real user/group has been changed by chown, this can be proved by mounting without the UID/GID options, as described further.

> Also, what if we mounted
> file system without specifying the UID/GID, then what UID/GID will be returned by
> your logic?

So this case is if I run “sudo mount /dev/sda1 /mnt”

Here the driver will not do any spoofing, and the real owners of the files shall be displayed. Thus running “ls -l” on a mounted partition without specifying UID/GID, files written by macOS shall be shown as 99 as the owner, iPadOS as 501, and if any file was written on Linux, the user who wrote it will be the owner.

If the user/group of any file was changed using chown, then the new user/group of the file will be displayed.
  

> On Dec 2, 2022, at 11:56 PM, Aditya Garg <gargaditya08@live.com> wrote:
> 
> 
>> Also, what if we mounted
>> file system without specifying the UID/GID, then what UID/GID will be returned by
>> your logic?
> 
> So this case is if I run “sudo mount /dev/sda1 /mnt”
> 
> Here the driver will not do any spoofing, and the real owners of the files shall be displayed. Thus running “ls -l” on a mounted partition without specifying UID/GID, files written by macOS shall be shown as 99 as the owner, iPadOS as 501, and if any file was written on Linux, the user who wrote it will be the owner.
> 
> If the user/group of any file was changed using chown, then the new user/group of the file will be displayed.

My question is much more simple.

diff --git a/fs/hfsplus/inode.c b/fs/hfsplus/inode.c
index aeab83ed1..4d1077db8 100644
--- a/fs/hfsplus/inode.c
+++ b/fs/hfsplus/inode.c
@@ -192,11 +192,11 @@ static void hfsplus_get_perms(struct inode *inode,
	mode = be16_to_cpu(perms->mode);

	i_uid_write(inode, be32_to_cpu(perms->owner));
-	if (!i_uid_read(inode) && !mode)
+	if (test_bit(HFSPLUS_SB_UID, &sbi->flags))
		inode->i_uid = sbi->uid;

	i_gid_write(inode, be32_to_cpu(perms->group));
-	if (!i_gid_read(inode) && !mode)
+	if (test_bit(HFSPLUS_SB_GID, &sbi->flags))
		inode->i_gid = sbi->gid;

Before this change, logic called i_uid_read(inode) and checked mode.
Now, we check only HFSPLUS_SB_UID/HFSPLUS_SB_GID flags.
So, if we mount HFS+ volume by “sudo mount /dev/sda1 /mnt”, then
HFSPLUS_SB_UID and HFSPLUS_SB_GID flags will be unset.
And current logic will do nothing. Is it correct logic? Maybe, we need
to use sbi->uid/gid if flag(s)HFSPLUS_SB_UID/HFSPLUS_SB_GID are set.
And if not, then to use old logic. Am I correct here? Or am I still missing
something here?

Thanks,
Slava.
  

> Before this change, logic called i_uid_read(inode) and checked mode.
> Now, we check only HFSPLUS_SB_UID/HFSPLUS_SB_GID flags.
> So, if we mount HFS+ volume by “sudo mount /dev/sda1 /mnt”, then
> HFSPLUS_SB_UID and HFSPLUS_SB_GID flags will be unset.
> And current logic will do nothing. Is it correct logic? Maybe, we need
> to use sbi->uid/gid if flag(s)HFSPLUS_SB_UID/HFSPLUS_SB_GID are set.
> And if not, then to use old logic. Am I correct here? Or am I still missing
> something here?

Well initially I when I tried to investigate what’s wrong, and found that the old logic was the culprit, I did some logging to see what exactly was wrong. The log patch is here btw :- https://github.com/AdityaGarg8/linux/commit/f668fb012f595d83053020b88b9439c295b4dc21

So I saw that the old logic was always false, no matter whether I mounted with uid or not.

I tried to see what makes this true, but couldn't succeed. So, I thought of a simpler approach and changed the logic itself.

To be honest, I dunno what is the old logic for. Maybe instead of completely removing the old logic, I could use an OR?

If you think its more logical, I can make this change :-

-	if (!i_gid_read(inode) && !mode)
+	if ((test_bit(HFSPLUS_SB_UID, &sbi->flags)) || (!i_uid_read(inode) && !mode))

Thanks
Aditya
  

> Well initially I when I tried to investigate what’s wrong, and found that the old logic was the culprit, I did some logging to see what exactly was wrong. The log patch is here btw :- https://github.com/AdityaGarg8/linux/commit/f668fb012f595d83053020b88b9439c295b4dc21
> 
> So I saw that the old logic was always false, no matter whether I mounted with uid or not.
> 
> I tried to see what makes this true, but couldn't succeed. So, I thought of a simpler approach and changed the logic itself.
> 
> To be honest, I dunno what is the old logic for. Maybe instead of completely removing the old logic, I could use an OR?
> 
> If you think its more logical, I can make this change :-
> 
> -	if (!i_gid_read(inode) && !mode)
> +	if ((test_bit(HFSPLUS_SB_UID, &sbi->flags)) || (!i_uid_read(inode) && !mode))
> 
> Thanks
> Aditya
> 
> 

I continuation with this message, I also think the bits should be set only if (!uid_valid(sbi->uid) is false, else the bits may be set even if UID is invalid? So, do you think the change given below should be good for this?

diff --git a/fs/hfsplus/options.c b/fs/hfsplus/options.c
index 047e05c57..c94a58762 100644
--- a/fs/hfsplus/options.c
+++ b/fs/hfsplus/options.c
@@ -140,6 +140,8 @@ int hfsplus_parse_options(char *input, struct hfsplus_sb_info *sbi)
 			if (!uid_valid(sbi->uid)) {
 				pr_err("invalid uid specified\n");
 				return 0;
+			} else {
+				set_bit(HFSPLUS_SB_UID, &sbi->flags);
 			}
 			break;
 		case opt_gid:
@@ -151,6 +153,8 @@ int hfsplus_parse_options(char *input, struct hfsplus_sb_info *sbi)
 			if (!gid_valid(sbi->gid)) {
 				pr_err("invalid gid specified\n");
 				return 0;
+			} else {
+				set_bit(HFSPLUS_SB_GID, &sbi->flags);
 			}
 			break;
 		case opt_part:
  

> On Dec 6, 2022, at 12:49 AM, Aditya Garg <gargaditya08@live.com> wrote:
> 
>> 
>> Well initially I when I tried to investigate what’s wrong, and found that the old logic was the culprit, I did some logging to see what exactly was wrong. The log patch is here btw :- https://github.com/AdityaGarg8/linux/commit/f668fb012f595d83053020b88b9439c295b4dc21
>> 
>> So I saw that the old logic was always false, no matter whether I mounted with uid or not.
>> 
>> I tried to see what makes this true, but couldn't succeed. So, I thought of a simpler approach and changed the logic itself.
>> 
>> To be honest, I dunno what is the old logic for. Maybe instead of completely removing the old logic, I could use an OR?
>> 
>> If you think its more logical, I can make this change :-
>> 
>> -	if (!i_gid_read(inode) && !mode)
>> +	if ((test_bit(HFSPLUS_SB_UID, &sbi->flags)) || (!i_uid_read(inode) && !mode))
>> 
>> Thanks
>> Aditya
>> 
>> 
> 
> I continuation with this message, I also think the bits should be set only if (!uid_valid(sbi->uid) is false, else the bits may be set even if UID is invalid? So, do you think the change given below should be good for this?
> 
> diff --git a/fs/hfsplus/options.c b/fs/hfsplus/options.c
> index 047e05c57..c94a58762 100644
> --- a/fs/hfsplus/options.c
> +++ b/fs/hfsplus/options.c
> @@ -140,6 +140,8 @@ int hfsplus_parse_options(char *input, struct hfsplus_sb_info *sbi)
> 			if (!uid_valid(sbi->uid)) {
> 				pr_err("invalid uid specified\n");
> 				return 0;
> +			} else {
> +				set_bit(HFSPLUS_SB_UID, &sbi->flags);
> 			}
> 			break;
> 		case opt_gid:
> @@ -151,6 +153,8 @@ int hfsplus_parse_options(char *input, struct hfsplus_sb_info *sbi)
> 			if (!gid_valid(sbi->gid)) {
> 				pr_err("invalid gid specified\n");
> 				return 0;
> +			} else {
> +				set_bit(HFSPLUS_SB_GID, &sbi->flags);
> 			}
> 			break;
> 		case opt_part:

Looks reasonably well. I believe it’s better fix.

Thanks,
Slava.
  

> Looks reasonably well. I believe it’s better fix.
> 
> Thanks,
> Slava.

Sending a version 2. I also forgot to Cc to stable, so shall do that in v2.