linux-kernel - Re: [PATCH v4 5/6] add listmount(2) syscall

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Thu, 11 Jan 2024 10:57:17 -0800
From: Guenter Roeck <linux@...ck-us.net>
To: Linus Torvalds <torvalds@...ux-foundation.org>
Cc: Miklos Szeredi <mszeredi@...hat.com>, linux-fsdevel@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-api@...r.kernel.org,
 linux-man@...r.kernel.org, linux-security-module@...r.kernel.org,
 Karel Zak <kzak@...hat.com>, Ian Kent <raven@...maw.net>,
 David Howells <dhowells@...hat.com>, Al Viro <viro@...iv.linux.org.uk>,
 Christian Brauner <christian@...uner.io>, Amir Goldstein
 <amir73il@...il.com>, Matthew House <mattlloydhouse@...il.com>,
 Florian Weimer <fweimer@...hat.com>, Arnd Bergmann <arnd@...db.de>
Subject: Re: [PATCH v4 5/6] add listmount(2) syscall

On 1/10/24 16:32, Linus Torvalds wrote:
> On Wed, 10 Jan 2024 at 14:23, Guenter Roeck <linux@...ck-us.net> wrote:
>>
>> with this patch in the tree, all sh4 builds fail with ICE.
>>
>> during RTL pass: final
>> In file included from fs/namespace.c:11:
>> fs/namespace.c: In function '__se_sys_listmount':
>> include/linux/syscalls.h:258:9: internal compiler error: in change_address_1, at emit-rtl.c:2275
> 
> We do have those very ugly SYSCALL_DEFINEx() macros, but I'm not
> seeing _anything_ that would be odd about the listmount case.
> 
> And the "__se_sys" thing in particular is just a fairly trivial wrapper.
> 
> It does use that asmlinkage_protect() thing, and it is unquestionably
> horrendously ugly (staring too long at <linux/syscalls.h> has been
> known to cause madness and despair), but we do that for *every* single
> system call and I don't see why the new listmount entry would be
> different.
> 

It isn't the syscall. The following hack avoids the problem.

diff --git a/fs/namespace.c b/fs/namespace.c
index ef1fd6829814..28fe2a55bd94 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -5070,8 +5070,10 @@ static ssize_t do_listmount(struct mount *first, struct path *orig, u64 mnt_id,
                 ctr = array_index_nospec(ctr, bufsize);
                 if (put_user(r->mnt_id_unique, buf + ctr))
                         return -EFAULT;
+#if 0
                 if (check_add_overflow(ctr, 1, &ctr))
                         return -ERANGE;
+#endif

But it isn't check_add_overflow() either. This "helps" as well.

diff --git a/fs/namespace.c b/fs/namespace.c
index ef1fd6829814..b53cb2f13530 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -5068,8 +5068,10 @@ static ssize_t do_listmount(struct mount *first, struct path *orig, u64 mnt_id,
                 if (!is_path_reachable(r, r->mnt.mnt_root, orig))
                         continue;
                 ctr = array_index_nospec(ctr, bufsize);
+#if 0
                 if (put_user(r->mnt_id_unique, buf + ctr))
                         return -EFAULT;
+#endif
                 if (check_add_overflow(ctr, 1, &ctr))
                         return -ERANGE;


Any variance of put_user() with &buf[ctr] or buf + ctr fails
if ctr is a variable and permitted to be != 0. For example,
commenting out the call to check_add_overflow() and starting
the loop with ctr = 1 also triggers the problem, as does replacing
the call to check_add_overflow() with ctr++;. Using a temporary
variable such as in
	u64 __user *pbuf;
	...
	pbuf = buf + ctr;
	if (put_user(r->mnt_id_unique, pbuf))
                         return -EFAULT;

doesn't help either. But this does:

-               if (put_user(r->mnt_id_unique, buf + ctr))
+               if (put_user(r->mnt_id_unique, (u32 *)(buf + ctr)))

and "buf + 17" as well as "&buf[17]" work as well. Essentially,
every combination of "buf + ctr" or "&buf[ctr]" fails if buf
is u64* and ctr is a variable.

The following works. Would this be acceptable ?

diff --git a/fs/namespace.c b/fs/namespace.c
index ef1fd6829814..dc0f844205d9 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -5068,10 +5068,11 @@ static ssize_t do_listmount(struct mount *first, struct path *orig, u64 mnt_id,
                 if (!is_path_reachable(r, r->mnt.mnt_root, orig))
                         continue;
                 ctr = array_index_nospec(ctr, bufsize);
-               if (put_user(r->mnt_id_unique, buf + ctr))
+               if (put_user(r->mnt_id_unique, buf))
                         return -EFAULT;
                 if (check_add_overflow(ctr, 1, &ctr))
                         return -ERANGE;
+               buf++;
         }
         return ctr;
  }

Guenter