[Devel] [PATCH vz10 06/24] ve/fs/devmnt: fix interior kfree() of strsep cursor in ve_devmnt_check()

[Pavel Tikhomirov]

Same fix to "[PATCH v6.1 VZ10 2/2] ve/fs/devmnt: free the right pointer in ve_devmnt_check()"

I personally prefer my version in terms of concise comments, other
differences are not that important.

On 7/6/26 12:59, Konstantin Khorenko wrote:
> ve_devmnt_check() kstrdup()s the option string and then walks it with
> strsep(&tmp_options, ","). strsep() advances the pointer it is given past
> each returned token, so on the mismatch (-EPERM) path
> 
> 	kfree(tmp_options);
> 
> frees a pointer that points into the *middle* of the kstrdup() allocation
> (or is NULL if the rejected token had no following separator). SLUB's
> non-debug kfree() does no object-boundary check: it derives the cache from
> the page and links the interior address onto the freelist as if it were a
> valid object, corrupting the kmalloc-16 slab. A subsequent kmalloc() of
> that size then hands out an interior pointer; the corruption typically
> surfaces far away, e.g. as
> 
> 	usercopy: Kernel memory overwrite attempt detected to SLUB object
> 	'kmalloc-16' (offset 11, size 15)!  kernel BUG at mm/usercopy.c:102
> 
> in an unrelated kernfs write. KASAN pinpoints it directly:
> 
> 	BUG: KASAN: invalid-free in ve_devmnt_check+0x135
> 	  kfree <- ve_devmnt_check <- ve_devmnt_process
> 	Allocated by task N: kstrdup <- ve_devmnt_check
> 
> This is reachable by any container root via mount(2)/fsconfig() with mount
> data of the form "allowedopt,disallowedopt" for a device that has a
> ve.mount_opts entry, so an unprivileged CT can corrupt host kernel slab.
> 
> Walk a throwaway cursor and always kfree() the original allocation. This
> also fixes a leak on the success path, where strsep() leaves tmp_options
> NULL and the old code did kfree(NULL), leaking the buffer on every allowed
> container mount.
> 
> Fixes: 263467c864c5 ("ve/fs/devmnt: process mount options")
> Feature: fs/mnt: hidden and allowed mount options in CT
> https://virtuozzo.atlassian.net/browse/VSTOR-137234
> Signed-off-by: Konstantin Khorenko <khorenko at virtuozzo.com>
> ---
>  fs/namespace.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/namespace.c b/fs/namespace.c
> index 566f11a222fc..c04b9f05f79f 100644
> --- a/fs/namespace.c
> +++ b/fs/namespace.c
> @@ -3140,7 +3140,7 @@ static char *strstr_separated(char *haystack, char *needle, char sep)
>  static int ve_devmnt_check(char *options, char *allowed)
>  {
>  	char *p;
> -	char *tmp_options;
> +	char *tmp_options, *tmp;
>  
>  	if (!options || !*options)
>  		return 0;
> @@ -3153,7 +3153,16 @@ static int ve_devmnt_check(char *options, char *allowed)
>  	if (!tmp_options)
>  		return -ENOMEM;
>  
> -	while ((p = strsep(&tmp_options, ",")) != NULL) {
> +	/*
> +	 * strsep() advances the pointer it is given past each token, so walk a
> +	 * throwaway cursor and always free the original kstrdup() allocation.
> +	 * Freeing the advanced pointer would kfree() an address in the middle
> +	 * of the object (an interior/invalid free that corrupts the slab), and
> +	 * on the success path strsep() leaves the cursor NULL so the original
> +	 * buffer would leak.
> +	 */
> +	tmp = tmp_options;
> +	while ((p = strsep(&tmp, ",")) != NULL) {
>  		if (!*p)
>  			continue;
>  

-- 
Best regards, Pavel Tikhomirov
Senior Software Developer, Virtuozzo.