Re: [Patch 8/9]: vt: Enhancements to the VT ioctl interface

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Downgrading user-sized allocations from kvmalloc-backed routines to kmalloc_array breaks legitimate large requests and allows an unprivileged user to exhaust memory or flood the kernel log.
- [High] Missing compat ioctl handlers for the newly introduced `PIO_UNIMAP21` and `GIO_UNIMAP21` commands breaks UAPI for 32-bit applications.
- [High] Uninitialized kernel stack memory leaked to userspace when CONFIG_CONSOLE_TRANSLATIONS is disabled.
- [Medium] Required buffer size is silently truncated in the legacy 16-bit GET ioctl if the dictionary exceeds 65535 entries.
- [Low] Incompatible pointer types passed to fallback dummy functions break the build when CONFIG_CONSOLE_TRANSLATIONS is disabled.
--

commit 50062db5106d84e65d87483ac16b8d41dbdd6175
Author: Alan Mackenzie <acm@muc.de>

    vt: Enhancements to the VT ioctl interface
    
    Handle the current 16-bit P/GIO_UNIMAP ioctls by converting
    to/from 32-bit code-points/glyph numbers.  Add handling for new
    32-bit ioctls P/GIO_UNIMAP21.  Convert to and from __user in
    vt_ioctl.c rather than in vt.c or consolemap.c.

> diff --git a/drivers/tty/vt/vt_ioctl.c b/drivers/tty/vt/vt_ioctl.c
> index 28993a3d0acb6..05986430e1bb3 100644
> --- a/drivers/tty/vt/vt_ioctl.c
> +++ b/drivers/tty/vt/vt_ioctl.c

[ ... ]

> @@ -484,10 +484,155 @@ static int vt_k_ioctl(struct tty_struct *tty, unsigned int cmd,
>  	return 0;
>  }
>  
> +#ifdef CONFIG_FB_GLYPH_21BIT
> +static int vt__user_unipair_8_to_21(unsigned int ct,
> +				    struct unipair __user *entries,
> +				    struct unipair21 **tmp21_entries)
> +{
> +	struct unipair *tmp_entries;
> +	int i, ret = 0;
> +
> +	tmp_entries = kmalloc_array(ct, sizeof(struct unipair), GFP_KERNEL);
> +	if (!tmp_entries)
> +		return -ENOMEM;
> +	if (copy_from_user(tmp_entries, entries, ct * sizeof(struct unipair))) {
> +		ret = -EFAULT;
> +		goto free_tmp;
> +	}
> +	*tmp21_entries = kmalloc_array(ct, sizeof(struct unipair21), GFP_KERNEL);

[Severity: High]
Could these unbounded allocations fail or cause page allocator warnings? A full
legacy map (65535 entries) requires an order-7 allocation for `tmp21_entries`
(524KB), which might spuriously fail without a vmalloc fallback. Additionally,
does this allow an unprivileged user to exhaust memory or flood the kernel log
by providing large `ct` values without `__GFP_NOWARN`?

> +	if (!*tmp21_entries) {
> +		ret = -ENOMEM;
> +		goto free_tmp;
> +	}

[ ... ]

> +static inline int do_unimap_ioctl(int cmd, struct unimapdesc __user *user_ud,
> +		bool perm, struct vc_data *vc)
> +{
> +	struct unimapdesc tmp;
> +	struct unipair21 *tmp21_entries = NULL;
> +	int ret = 0, ret1 = 0;
> +	unsigned int ct;
> +
> +	if (copy_from_user(&tmp, user_ud, sizeof(tmp)))
> +		return -EFAULT;
> +	switch (cmd) {

[ ... ]

> +	case GIO_UNIMAP:
> +		if (!perm && fg_console != vc->vc_num)
> +			return -EPERM;
> +		tmp21_entries = kmalloc_array(tmp.entry_ct,
> +					      sizeof(struct unipair21),
> +					      GFP_KERNEL);
> +		if (!tmp21_entries)
> +			return -ENOMEM;
> +		ret = con_get_unimap(vc, tmp.entry_ct, &ct, tmp21_entries);
> +		if (ret)
> +			goto free_tmp21;
> +		ret = vt_unipair_21_to__user_8(ct, tmp21_entries, tmp.entries);
> +free_tmp21:	ret1 = put_user(ct, &user_ud->entry_ct);

[Severity: High]
If CONFIG_CONSOLE_TRANSLATIONS is disabled, the dummy `con_get_unimap` inline
returns `-EINVAL` without initializing `ct`. Does this result in uninitialized
kernel stack memory leaking to userspace when `ct` is copied by `put_user()`
on the error path?

[Severity: Medium]
Also, could this silently truncate the required buffer size if the dictionary
exceeds 65535 entries? The 32-bit accurate size `ct` is written to a 16-bit
`user_ud->entry_ct`, causing legacy applications reading the map via GIO_UNIMAP
to receive an incorrect, truncated size and fail to allocate a sufficient buffer.

> +		kfree(tmp21_entries);
> +		return ret ? ret : ret1;
> +	}
> +	return 0;
> +}
> +
> +static inline int do_unimap_ioctl21(int cmd, struct unimapdesc21 __user *user_ud,
> +		bool perm, struct vc_data *vc)
> +{
> +	struct unimapdesc21 tmp;
> +	struct unipair21 *k_entries;
> +	int ret = 0, ret1 = 0;
> +
> +	if (copy_from_user(&tmp, user_ud, sizeof(tmp)))
> +		return -EFAULT;
> +	switch (cmd) {
> +	case PIO_UNIMAP21:
> +		if (!perm)
> +			return -EPERM;
> +		k_entries = kmalloc_array(tmp.entry_ct,
> +					  sizeof(struct unipair21),
> +					  GFP_KERNEL);

[Severity: High]
Does this allow unbounded allocations similar to `vt__user_unipair_8_to_21()`?
Passing an unbounded user-provided size `tmp.entry_ct` to `kmalloc_array`
without `__GFP_NOWARN` could allow an unprivileged user to trigger page
allocator warnings and exhaust memory. Furthermore, `con_set_unimap()`
propagates this count to `dict->count`, leading to additional unbounded
`kmalloc` allocations in `set_inverse_transl()`.

[ ... ]

> @@ -1026,16 +1203,20 @@ compat_kdfontop_ioctl(struct compat_console_font_op __user *fontop,
>  }
>  
>  struct compat_unimapdesc {
> -	unsigned short entry_ct;
> +	u16 entry_ct;
>  	compat_caddr_t entries;
>  };
>  
> +#ifdef CONFIG_FB_GLYPH_21BIT
>  static inline int
>  compat_unimap_ioctl(unsigned int cmd, struct compat_unimapdesc __user *user_ud,
>  			 int perm, struct vc_data *vc)
>  {

[Severity: High]
Are we missing compat ioctl handlers for `PIO_UNIMAP21` and `GIO_UNIMAP21`?
`struct unimapdesc21` contains a pointer, so its layout differs between 32-bit
and 64-bit architectures (8 bytes vs 16 bytes). Without explicit handling in
`vt_compat_ioctl()`, 32-bit processes on 64-bit kernels will fail to invoke
these new ioctls and receive -ENOIOCTLCMD or -ENOTTY.

[ ... ]

> diff --git a/include/linux/vt_kern.h b/include/linux/vt_kern.h
> index e017588979194..af99aeefe177f 100644
> --- a/include/linux/vt_kern.h
> +++ b/include/linux/vt_kern.h
> @@ -96,13 +96,13 @@ static inline int con_clear_unimap(struct vc_data *vc)
>  	return 0;
>  }
>  static inline
> -int con_set_unimap(struct vc_data *vc, ushort ct, struct unipair __user *list)
> +int con_set_unimap(struct vc_data *vc, ushort ct, struct unipair *list)

[Severity: Low]
Will this break the build when CONFIG_CONSOLE_TRANSLATIONS is disabled?
The fallback dummy function for `con_set_unimap` expects a `struct unipair *`,
but the callers in `vt_ioctl.c` under `CONFIG_FB_GLYPH_21BIT` (like
`do_unimap_ioctl21`) pass `struct unipair21 *`, which violates strict type
checking.

>  {
>  	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/apCEDM2sWv_M354-@MAC.fritz.box?part=8