| Message ID | 7cd32f988a147d7617742c9e074c753de0c6bc1f.1712080158.git.legion@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | VT: Add ability to get font requirements | expand |
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
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,
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.
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 --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 */