diff mbox series

[v4,2/3] VT: Add KDFONTINFO ioctl

Message ID 7cd32f988a147d7617742c9e074c753de0c6bc1f.1712080158.git.legion@kernel.org
State New
Headers show
Series VT: Add ability to get font requirements | expand

Commit Message

Alexey Gladkov April 2, 2024, 5:50 p.m. UTC 
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 minimum and maximum font size values.

Acked-by: Helge Deller <deller@gmx.de>
Signed-off-by: Alexey Gladkov <legion@kernel.org>
---
 drivers/tty/vt/vt.c       | 24 ++++++++++++++++++++++++
 drivers/tty/vt/vt_ioctl.c | 11 +++++++++++
 include/linux/console.h   |  3 +++
 include/linux/vt_kern.h   |  1 +
 include/uapi/linux/kd.h   | 13 ++++++++++++-
 5 files changed, 51 insertions(+), 1 deletion(-)

Comments

Greg KH April 3, 2024, 4:55 a.m. UTC | #1 
On Tue, Apr 02, 2024 at 07:50:45PM +0200, Alexey Gladkov wrote:
> +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_* */
> +};

As Jiri said, this will not work for an ioctl structure at all, sorry.
Please read the kernel documentation about how to write a new ioctl for
how to do this correctly (hint, you can not use 'unsigned int' in a
structure that crosses the kernel/user boundry for new ioctls.)

thanks,

greg k-h
Jiri Slaby (SUSE) April 3, 2024, 5:05 a.m. UTC | #2 
First, there was no need to send this v4 so quickly. Provided we have 
not settled in v3... This makes the review process painful.

And then:

On 02. 04. 24, 19:50, 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 minimum and maximum font size values.
> 
> Acked-by: Helge Deller <deller@gmx.de>
> Signed-off-by: Alexey Gladkov <legion@kernel.org>
...
> --- a/drivers/tty/vt/vt_ioctl.c
> +++ b/drivers/tty/vt/vt_ioctl.c
> @@ -479,6 +479,17 @@ static int vt_k_ioctl(struct tty_struct *tty, unsigned int cmd,
>   		break;
>   	}
>   
> +	case KDFONTINFO: {
> +		struct console_font_info fnt_info;
> +
> +		ret = con_font_info(vc, &fnt_info);
> +		if (ret)
> +			return ret;
> +		if (copy_to_user(up, &fnt_info, sizeof(fnt_info)))

sizeof, I already commented.

Now you leak info to userspace unless everyone sets everything in 
fnt_info. IOW, do memset() above.

> +			return -EFAULT;
> +		break;
> +	}
> +
>   	default:
>   		return -ENOIOCTLCMD;
>   	}

thanks,
Alexey Gladkov April 10, 2024, 4:36 p.m. UTC | #3 
On Wed, Apr 03, 2024 at 07:05:14AM +0200, Jiri Slaby wrote:
> First, there was no need to send this v4 so quickly. Provided we have 
> not settled in v3... This makes the review process painful.
> 
> And then:
> 
> On 02. 04. 24, 19:50, 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 minimum and maximum font size values.
> > 
> > Acked-by: Helge Deller <deller@gmx.de>
> > Signed-off-by: Alexey Gladkov <legion@kernel.org>
> ...
> > --- a/drivers/tty/vt/vt_ioctl.c
> > +++ b/drivers/tty/vt/vt_ioctl.c
> > @@ -479,6 +479,17 @@ static int vt_k_ioctl(struct tty_struct *tty, unsigned int cmd,
> >   		break;
> >   	}
> >   
> > +	case KDFONTINFO: {
> > +		struct console_font_info fnt_info;
> > +
> > +		ret = con_font_info(vc, &fnt_info);
> > +		if (ret)
> > +			return ret;
> > +		if (copy_to_user(up, &fnt_info, sizeof(fnt_info)))
> 
> sizeof, I already commented.

I'm not sure I understand. sizeof(*up), but 'up' is 'void __user *up'.

> Now you leak info to userspace unless everyone sets everything in 
> fnt_info. IOW, do memset() above.

Yes. I miss it. Sorry.
Jiri Slaby (SUSE) April 11, 2024, 3:53 a.m. UTC | #4 
On 10. 04. 24, 18:36, Alexey Gladkov wrote:
> On Wed, Apr 03, 2024 at 07:05:14AM +0200, Jiri Slaby wrote:
>> First, there was no need to send this v4 so quickly. Provided we have
>> not settled in v3... This makes the review process painful.
>>
>> And then:
>>
>> On 02. 04. 24, 19:50, 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 minimum and maximum font size values.
>>>
>>> Acked-by: Helge Deller <deller@gmx.de>
>>> Signed-off-by: Alexey Gladkov <legion@kernel.org>
>> ...
>>> --- a/drivers/tty/vt/vt_ioctl.c
>>> +++ b/drivers/tty/vt/vt_ioctl.c
>>> @@ -479,6 +479,17 @@ static int vt_k_ioctl(struct tty_struct *tty, unsigned int cmd,
>>>    		break;
>>>    	}
>>>    
>>> +	case KDFONTINFO: {
>>> +		struct console_font_info fnt_info;
>>> +
>>> +		ret = con_font_info(vc, &fnt_info);
>>> +		if (ret)
>>> +			return ret;
>>> +		if (copy_to_user(up, &fnt_info, sizeof(fnt_info)))
>>
>> sizeof, I already commented.
> 
> I'm not sure I understand. sizeof(*up), but 'up' is 'void __user *up'.

I don't know what I saw/was thinking previously, ignore this one :).

thanks,
diff mbox series

Patch

diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
index 9b5b98dfc8b4..e8db0e9ea674 100644
--- a/drivers/tty/vt/vt.c
+++ b/drivers/tty/vt/vt.c
@@ -4851,6 +4851,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;
+
+	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 4b91072f3a4e..40f9467f503d 100644
--- a/drivers/tty/vt/vt_ioctl.c
+++ b/drivers/tty/vt/vt_ioctl.c
@@ -479,6 +479,17 @@  static int vt_k_ioctl(struct tty_struct *tty, unsigned int cmd,
 		break;
 	}
 
+	case KDFONTINFO: {
+		struct console_font_info fnt_info;
+
+		ret = con_font_info(vc, &fnt_info);
+		if (ret)
+			return ret;
+		if (copy_to_user(up, &fnt_info, sizeof(fnt_info)))
+			return -EFAULT;
+		break;
+	}
+
 	default:
 		return -ENOIOCTLCMD;
 	}
diff --git a/include/linux/console.h b/include/linux/console.h
index 31a8f5b85f5d..4b798322aa01 100644
--- a/include/linux/console.h
+++ b/include/linux/console.h
@@ -21,6 +21,7 @@ 
 #include <linux/vesa.h>
 
 struct vc_data;
+struct console_font_info;
 struct console_font_op;
 struct console_font;
 struct module;
@@ -102,6 +103,8 @@  struct consw {
 	bool	(*con_switch)(struct vc_data *vc);
 	bool	(*con_blank)(struct vc_data *vc, enum vesa_blank_mode blank,
 			     bool mode_switch);
+	int	(*con_font_info)(struct vc_data *vc,
+				 struct console_font_info *info);
 	int	(*con_font_set)(struct vc_data *vc,
 				const struct console_font *font,
 				unsigned int vpitch, unsigned int flags);
diff --git a/include/linux/vt_kern.h b/include/linux/vt_kern.h
index d008c3d0a9bb..383b3a4f6113 100644
--- a/include/linux/vt_kern.h
+++ b/include/linux/vt_kern.h
@@ -33,6 +33,7 @@  void do_blank_screen(int entering_gfx);
 void do_unblank_screen(int leaving_gfx);
 void poke_blanked_console(void);
 int con_font_op(struct vc_data *vc, struct console_font_op *op);
+int con_font_info(struct vc_data *vc, struct console_font_info *info);
 int con_set_cmap(unsigned char __user *cmap);
 int con_get_cmap(unsigned char __user *cmap);
 void scrollback(struct vc_data *vc);
diff --git a/include/uapi/linux/kd.h b/include/uapi/linux/kd.h
index 8ddb2219a84b..abaf4dd6bb93 100644
--- a/include/uapi/linux/kd.h
+++ b/include/uapi/linux/kd.h
@@ -185,8 +185,19 @@  struct console_font {
 
 #define KD_FONT_FLAG_DONT_RECALC 	1	/* Don't recalculate hw charcell size [compat] */
 
+#define KDFONTINFO	_IO(KD_IOCTL_BASE, 0x73)	/* font information */
+
+#define KD_FONT_INFO_FLAG_LOW_SIZE	_BITUL(0) /* 256 */
+#define KD_FONT_INFO_FLAG_HIGH_SIZE	_BITUL(1) /* 512 */
+
+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_* */
+};
+
 /* note: 0x4B00-0x4B4E all have had a value at some time;
    don't reuse for the time being */
-/* note: 0x4B60-0x4B6D, 0x4B70-0x4B72 used above */
+/* note: 0x4B60-0x4B6D, 0x4B70-0x4B73 used above */
 
 #endif /* _UAPI_LINUX_KD_H */