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

[Guenter Roeck <linux@roeck-us.net>]

On 1/10/24 16:32, Linus Torvalds wrote:
> On Wed, 10 Jan 2024 at 14:23, Guenter Roeck <linux@roeck-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