[PATCH] fs: reject U64_MAX as last_mnt_id in listmount()

[PATCH] fs: reject U64_MAX as last_mnt_id in listmount()

[Tao Cui]

From: Tao Cui <cuitao@kylinos.cn>

listmount() uses mnt_id_req::param as a pagination cursor: on forward
iteration do_listmount() starts from the first mount with an id strictly
greater than last_mnt_id.  The sanity check in prepare_klistmount() only
rejects ids in the range [1, MNT_UNIQUE_ID_OFFSET]; U64_MAX passes the
check, and incrementing it wraps back to 0, so mnt_find_id_at() restarts
from the leftmost mount in the namespace every time.

A caller paging with last_mnt_id set to U64_MAX therefore always gets
the same first batch of ids returned and can never advance to the end of
the list:

  # base: every call returns the same first batch
  listmount(param=U64_MAX) = 8 ids: 2147483886 2147483888 ...
  # patched:
  listmount(param=U64_MAX) = -1 EINVAL

Reject U64_MAX up front together with the other invalid ids.

Fixes: b4c2bea8ceaa ("add listmount(2) syscall")
Cc: stable@vger.kernel.org
Signed-off-by: Tao Cui <cuitao@kylinos.cn>
---
 fs/namespace.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/namespace.c b/fs/namespace.c
index 8d4009cfbf5b..cb95b855f78b 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -6108,6 +6108,9 @@ static inline int prepare_klistmount(struct klistmount *kls, struct mnt_id_req *
 	/* The first valid unique mount id is MNT_UNIQUE_ID_OFFSET + 1. */
 	if (last_mnt_id != 0 && last_mnt_id <= MNT_UNIQUE_ID_OFFSET)
 		return -EINVAL;
+	/* U64_MAX would wrap to 0 and restart the iteration. */
+	if (last_mnt_id == U64_MAX)
+		return -EINVAL;
 
 	kls->last_mnt_id = last_mnt_id;
 
-- 
2.43.0

Re: [PATCH] fs: reject U64_MAX as last_mnt_id in listmount()

[Jan Kara]

On Mon 07-09-26 14:57:23, Tao Cui wrote:
> From: Tao Cui <cuitao@kylinos.cn>
> 
> listmount() uses mnt_id_req::param as a pagination cursor: on forward
> iteration do_listmount() starts from the first mount with an id strictly
> greater than last_mnt_id.  The sanity check in prepare_klistmount() only
> rejects ids in the range [1, MNT_UNIQUE_ID_OFFSET]; U64_MAX passes the
> check, and incrementing it wraps back to 0, so mnt_find_id_at() restarts
> from the leftmost mount in the namespace every time.
> 
> A caller paging with last_mnt_id set to U64_MAX therefore always gets
> the same first batch of ids returned and can never advance to the end of
> the list:
> 
>   # base: every call returns the same first batch
>   listmount(param=U64_MAX) = 8 ids: 2147483886 2147483888 ...
>   # patched:
>   listmount(param=U64_MAX) = -1 EINVAL
> 
> Reject U64_MAX up front together with the other invalid ids.
> 
> Fixes: b4c2bea8ceaa ("add listmount(2) syscall")
> Cc: stable@vger.kernel.org
> Signed-off-by: Tao Cui <cuitao@kylinos.cn>

This looks OK, but I think we should reject U64_MAX only if
LISTMOUNT_REVERSE flag is not set? With this flag specifying U64_MAX
actually makes sense and it is likely used by people?

								Honza

> ---
>  fs/namespace.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/fs/namespace.c b/fs/namespace.c
> index 8d4009cfbf5b..cb95b855f78b 100644
> --- a/fs/namespace.c
> +++ b/fs/namespace.c
> @@ -6108,6 +6108,9 @@ static inline int prepare_klistmount(struct klistmount *kls, struct mnt_id_req *
>  	/* The first valid unique mount id is MNT_UNIQUE_ID_OFFSET + 1. */
>  	if (last_mnt_id != 0 && last_mnt_id <= MNT_UNIQUE_ID_OFFSET)
>  		return -EINVAL;
> +	/* U64_MAX would wrap to 0 and restart the iteration. */
> +	if (last_mnt_id == U64_MAX)
> +		return -EINVAL;
>  
>  	kls->last_mnt_id = last_mnt_id;
>  
> -- 
> 2.43.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR