Re: [RESEND PATCH v3 1/2] VT: Add KDFONTINFO ioctl

From: Jiri Slaby
Date: Tue Apr 02 2024 - 07:03:40 EST


Hi,

On 02. 04. 24, 12:32, Alexey Gladkov wrote:
Each driver has its own restrictions on font size. There is currently no
way to understand what the requirements are. The new ioctl allows
userspace to get the minmum and maximum font size values.

minimum

Acked-by: Helge Deller <deller@xxxxxx>
Signed-off-by: Alexey Gladkov <legion@xxxxxxxxxx>
---
drivers/tty/vt/vt.c | 24 ++++++++++++++++++++++++
drivers/tty/vt/vt_ioctl.c | 13 +++++++++++++
include/linux/console.h | 2 ++
include/linux/vt_kern.h | 1 +
include/uapi/linux/kd.h | 13 ++++++++++++-
5 files changed, 52 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
index 156efda7c80d..8c2a3d98b5ec 100644
--- a/drivers/tty/vt/vt.c
+++ b/drivers/tty/vt/vt.c
@@ -4680,6 +4680,30 @@ int con_font_op(struct vc_data *vc, struct console_font_op *op)
return -ENOSYS;
}
+int con_font_info(struct vc_data *vc, struct console_font_info *info)
+{
+ int rc = -EINVAL;

This initialization appears to be unneeded.

+
+ info->min_height = 0;
+ info->max_height = max_font_height;
+
+ info->min_width = 0;
+ info->max_width = max_font_width;
+
+ info->flags = KD_FONT_INFO_FLAG_LOW_SIZE | KD_FONT_INFO_FLAG_HIGH_SIZE;
+
+ console_lock();
+ if (vc->vc_mode != KD_TEXT)
+ rc = -EINVAL;
+ else if (vc->vc_sw->con_font_info)
+ rc = vc->vc_sw->con_font_info(vc, info);
+ else
+ rc = -ENOSYS;
+ console_unlock();
+
+ return rc;
+}
+
/*
* Interface exported to selection and vcs.
*/
diff --git a/drivers/tty/vt/vt_ioctl.c b/drivers/tty/vt/vt_ioctl.c
index 8c685b501404..b3b4e4b69366 100644
--- a/drivers/tty/vt/vt_ioctl.c
+++ b/drivers/tty/vt/vt_ioctl.c
@@ -479,6 +479,19 @@ static int vt_k_ioctl(struct tty_struct *tty, unsigned int cmd,
break;
}
+ case KDFONTINFO: {
+ struct console_font_info fnt_info;
+
+ if (copy_from_user(&fnt_info, up, sizeof(fnt_info)))
+ return -EFAULT;

Who uses the copied values?

+ ret = con_font_info(vc, &fnt_info);
+ if (ret)
+ return ret;
+ if (copy_to_user(up, &fnt_info, sizeof(fnt_info)))

We should do the preferred sizeof(*up) here...

+ return -EFAULT;
+ break;
+ }
+
default:
return -ENOIOCTLCMD;
}
..
--- a/include/uapi/linux/kd.h
+++ b/include/uapi/linux/kd.h
@@ -183,8 +183,19 @@ struct console_font {
#define KD_FONT_FLAG_DONT_RECALC 1 /* Don't recalculate hw charcell size [compat] */
+#define KDFONTINFO 0x4B73 /* font information */

Why not properly define the number using IOC() et al.? K (that 0x4b) is even reserved for kd.h.

+#define KD_FONT_INFO_FLAG_LOW_SIZE (1U << 0) /* 256 */
+#define KD_FONT_INFO_FLAG_HIGH_SIZE (1U << 1) /* 512 */

_BITUL()

+struct console_font_info {
+ unsigned int min_width, min_height; /* minimal font size */
+ unsigned int max_width, max_height; /* maximum font size */
+ unsigned int flags; /* KD_FONT_INFO_FLAG_* */

This does not look like a well-defined™ and extendable uapi structure. While it won't change anything here, still use fixed-length __u32.

And you should perhaps add some reserved fields. Do not repeat the same mistakes as your predecessors with the current kd uapi.

+};

thanks,
--
js
suse labs