`struct console_font` is a UAPI structure, thus ideally should not be
used for kernel internal abstraction. Remove some dummy .con_font_set,
.con_font_default and .con_font_copy `struct consw` callback
implementations, to make it cleaner.
Patch "fbcon: Prevent global-out-of-bounds read in fbcon_copy_font()"
depends on this patch, so Cc: stable.
Cc: [email protected]
Suggested-by: Daniel Vetter <[email protected]>
Signed-off-by: Peilin Ye <[email protected]>
---
Context: https://lore.kernel.org/lkml/CAKMK7uFY2zv0adjKJ_ORVFT7Zzwn075MaU0rEU7_FuqENLR=UA@mail.gmail.com/
drivers/usb/misc/sisusbvga/sisusb_con.c | 21 ---------------------
drivers/video/console/dummycon.c | 20 --------------------
2 files changed, 41 deletions(-)
diff --git a/drivers/usb/misc/sisusbvga/sisusb_con.c b/drivers/usb/misc/sisusbvga/sisusb_con.c
index c63e545fb105..dfa0d5ce6012 100644
--- a/drivers/usb/misc/sisusbvga/sisusb_con.c
+++ b/drivers/usb/misc/sisusbvga/sisusb_con.c
@@ -1345,24 +1345,6 @@ static int sisusbdummycon_blank(struct vc_data *vc, int blank, int mode_switch)
return 0;
}
-static int sisusbdummycon_font_set(struct vc_data *vc,
- struct console_font *font,
- unsigned int flags)
-{
- return 0;
-}
-
-static int sisusbdummycon_font_default(struct vc_data *vc,
- struct console_font *font, char *name)
-{
- return 0;
-}
-
-static int sisusbdummycon_font_copy(struct vc_data *vc, int con)
-{
- return 0;
-}
-
static const struct consw sisusb_dummy_con = {
.owner = THIS_MODULE,
.con_startup = sisusbdummycon_startup,
@@ -1375,9 +1357,6 @@ static const struct consw sisusb_dummy_con = {
.con_scroll = sisusbdummycon_scroll,
.con_switch = sisusbdummycon_switch,
.con_blank = sisusbdummycon_blank,
- .con_font_set = sisusbdummycon_font_set,
- .con_font_default = sisusbdummycon_font_default,
- .con_font_copy = sisusbdummycon_font_copy,
};
int
diff --git a/drivers/video/console/dummycon.c b/drivers/video/console/dummycon.c
index 2a0d0bda7faa..f1711b2f9ff0 100644
--- a/drivers/video/console/dummycon.c
+++ b/drivers/video/console/dummycon.c
@@ -124,23 +124,6 @@ static int dummycon_switch(struct vc_data *vc)
return 0;
}
-static int dummycon_font_set(struct vc_data *vc, struct console_font *font,
- unsigned int flags)
-{
- return 0;
-}
-
-static int dummycon_font_default(struct vc_data *vc,
- struct console_font *font, char *name)
-{
- return 0;
-}
-
-static int dummycon_font_copy(struct vc_data *vc, int con)
-{
- return 0;
-}
-
/*
* The console `switch' structure for the dummy console
*
@@ -159,8 +142,5 @@ const struct consw dummy_con = {
.con_scroll = dummycon_scroll,
.con_switch = dummycon_switch,
.con_blank = dummycon_blank,
- .con_font_set = dummycon_font_set,
- .con_font_default = dummycon_font_default,
- .con_font_copy = dummycon_font_copy,
};
EXPORT_SYMBOL_GPL(dummy_con);
--
2.25.1
fbcon_copy_font() is using a signed int, `con`, as an index into
`fb_display[MAX_NR_CONSOLES]`, without bounds checking. In
con_font_copy(), `con` is being silently casted from the unsigned
`op->height`.
Let con_font_copy() and fbcon_copy_font() pass `op->height` directly, and
add a range check in fbcon_copy_font(). Also, add a comment in
con_font_op() for less confusion, since ideally `op->height` should not be
used as a console index, as the field name suggests.
This patch depends on patch "console: Remove dummy con_font_op callback
implementations".
Cc: [email protected]
Signed-off-by: Peilin Ye <[email protected]>
---
drivers/tty/vt/vt.c | 6 +++---
drivers/video/fbdev/core/fbcon.c | 8 ++++++--
include/linux/console.h | 2 +-
3 files changed, 10 insertions(+), 6 deletions(-)
diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
index 9506a76f3ab6..ff8ea1654a69 100644
--- a/drivers/tty/vt/vt.c
+++ b/drivers/tty/vt/vt.c
@@ -4704,9 +4704,8 @@ static int con_font_default(struct vc_data *vc, struct console_font_op *op)
return rc;
}
-static int con_font_copy(struct vc_data *vc, struct console_font_op *op)
+static int con_font_copy(struct vc_data *vc, unsigned int con)
{
- int con = op->height;
int rc;
@@ -4735,7 +4734,8 @@ int con_font_op(struct vc_data *vc, struct console_font_op *op)
case KD_FONT_OP_SET_DEFAULT:
return con_font_default(vc, op);
case KD_FONT_OP_COPY:
- return con_font_copy(vc, op);
+ /* uses op->height as a console index */
+ return con_font_copy(vc, op->height);
}
return -ENOSYS;
}
diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
index cef437817b0d..1caa98146712 100644
--- a/drivers/video/fbdev/core/fbcon.c
+++ b/drivers/video/fbdev/core/fbcon.c
@@ -2451,11 +2451,15 @@ static int fbcon_do_set_font(struct vc_data *vc, int w, int h,
return 0;
}
-static int fbcon_copy_font(struct vc_data *vc, int con)
+static int fbcon_copy_font(struct vc_data *vc, unsigned int con)
{
- struct fbcon_display *od = &fb_display[con];
+ struct fbcon_display *od;
struct console_font *f = &vc->vc_font;
+ if (con >= MAX_NR_CONSOLES)
+ return -EINVAL;
+
+ od = &fb_display[con];
if (od->fontdata == f->data)
return 0; /* already the same font... */
return fbcon_do_set_font(vc, f->width, f->height, od->fontdata, od->userfont);
diff --git a/include/linux/console.h b/include/linux/console.h
index 4b1e26c4cb42..34855d3f2afd 100644
--- a/include/linux/console.h
+++ b/include/linux/console.h
@@ -62,7 +62,7 @@ struct consw {
int (*con_font_get)(struct vc_data *vc, struct console_font *font);
int (*con_font_default)(struct vc_data *vc,
struct console_font *font, char *name);
- int (*con_font_copy)(struct vc_data *vc, int con);
+ int (*con_font_copy)(struct vc_data *vc, unsigned int con);
int (*con_resize)(struct vc_data *vc, unsigned int width,
unsigned int height, unsigned int user);
void (*con_set_palette)(struct vc_data *vc,
--
2.25.1
`struct console_font` is a UAPI structure, thus ideally should not be
used for kernel internal abstraction. Remove some dummy .con_font_set,
.con_font_default and .con_font_copy `struct consw` callback
implementations, to make it cleaner.
Suggested-by: Daniel Vetter <[email protected]>
Signed-off-by: Peilin Ye <[email protected]>
---
Change in v2:
- [v2 2/2] no longer Cc: stable, so do not Cc: stable
Context: https://lore.kernel.org/lkml/CAKMK7uFY2zv0adjKJ_ORVFT7Zzwn075MaU0rEU7_FuqENLR=UA@mail.gmail.com/
drivers/usb/misc/sisusbvga/sisusb_con.c | 21 ---------------------
drivers/video/console/dummycon.c | 20 --------------------
2 files changed, 41 deletions(-)
diff --git a/drivers/usb/misc/sisusbvga/sisusb_con.c b/drivers/usb/misc/sisusbvga/sisusb_con.c
index c63e545fb105..dfa0d5ce6012 100644
--- a/drivers/usb/misc/sisusbvga/sisusb_con.c
+++ b/drivers/usb/misc/sisusbvga/sisusb_con.c
@@ -1345,24 +1345,6 @@ static int sisusbdummycon_blank(struct vc_data *vc, int blank, int mode_switch)
return 0;
}
-static int sisusbdummycon_font_set(struct vc_data *vc,
- struct console_font *font,
- unsigned int flags)
-{
- return 0;
-}
-
-static int sisusbdummycon_font_default(struct vc_data *vc,
- struct console_font *font, char *name)
-{
- return 0;
-}
-
-static int sisusbdummycon_font_copy(struct vc_data *vc, int con)
-{
- return 0;
-}
-
static const struct consw sisusb_dummy_con = {
.owner = THIS_MODULE,
.con_startup = sisusbdummycon_startup,
@@ -1375,9 +1357,6 @@ static const struct consw sisusb_dummy_con = {
.con_scroll = sisusbdummycon_scroll,
.con_switch = sisusbdummycon_switch,
.con_blank = sisusbdummycon_blank,
- .con_font_set = sisusbdummycon_font_set,
- .con_font_default = sisusbdummycon_font_default,
- .con_font_copy = sisusbdummycon_font_copy,
};
int
diff --git a/drivers/video/console/dummycon.c b/drivers/video/console/dummycon.c
index 2a0d0bda7faa..f1711b2f9ff0 100644
--- a/drivers/video/console/dummycon.c
+++ b/drivers/video/console/dummycon.c
@@ -124,23 +124,6 @@ static int dummycon_switch(struct vc_data *vc)
return 0;
}
-static int dummycon_font_set(struct vc_data *vc, struct console_font *font,
- unsigned int flags)
-{
- return 0;
-}
-
-static int dummycon_font_default(struct vc_data *vc,
- struct console_font *font, char *name)
-{
- return 0;
-}
-
-static int dummycon_font_copy(struct vc_data *vc, int con)
-{
- return 0;
-}
-
/*
* The console `switch' structure for the dummy console
*
@@ -159,8 +142,5 @@ const struct consw dummy_con = {
.con_scroll = dummycon_scroll,
.con_switch = dummycon_switch,
.con_blank = dummycon_blank,
- .con_font_set = dummycon_font_set,
- .con_font_default = dummycon_font_default,
- .con_font_copy = dummycon_font_copy,
};
EXPORT_SYMBOL_GPL(dummy_con);
--
2.25.1
con_font_op() is passing an entire `struct console_font_op *` to
con_font_copy(), but con_font_copy() only uses `op->height`. Additionally,
con_font_copy() is silently assigning the unsigned `op->height` to the
signed `con`, then pass it to fbcon_copy_font().
Let con_font_copy() and fbcon_copy_font() pass an unsigned int directly.
Also, add a comment in con_font_op() for less confusion, since ideally
`op->height` should not be used as a console index, as the field name
suggests.
This patch depends on patch "console: Remove dummy con_font_op() callback
implementations".
Suggested-by: Daniel Vetter <[email protected]>
Signed-off-by: Peilin Ye <[email protected]>
---
con_font_set(), con_font_get() and con_font_default() also pass an entire
`console_font_op`.
con_font_get() and con_font_default() actually update the structure (later
copied to userspace), so let them be.
con_font_set() does not update the structure, but it uses all fields of it
except `op`. Avoiding passing `console_font_op` to con_font_set() will
thus make its signature pretty long (6 parameters).
Changes in v2:
- Remove redundant `con < 0` check in con_font_copy() (kernel test robot
<[email protected]>)
- Remove unnecessary range check in fbcon_copy_font(). con_font_copy()
calls vc_cons_allocated(), which does the check
- Do not Cc: stable
- Rewrite the title and commit message accordingly
drivers/tty/vt/vt.c | 8 ++++----
drivers/video/fbdev/core/fbcon.c | 2 +-
include/linux/console.h | 2 +-
3 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
index 9506a76f3ab6..27821ef97b13 100644
--- a/drivers/tty/vt/vt.c
+++ b/drivers/tty/vt/vt.c
@@ -4704,9 +4704,8 @@ static int con_font_default(struct vc_data *vc, struct console_font_op *op)
return rc;
}
-static int con_font_copy(struct vc_data *vc, struct console_font_op *op)
+static int con_font_copy(struct vc_data *vc, unsigned int con)
{
- int con = op->height;
int rc;
@@ -4715,7 +4714,7 @@ static int con_font_copy(struct vc_data *vc, struct console_font_op *op)
rc = -EINVAL;
else if (!vc->vc_sw->con_font_copy)
rc = -ENOSYS;
- else if (con < 0 || !vc_cons_allocated(con))
+ else if (!vc_cons_allocated(con))
rc = -ENOTTY;
else if (con == vc->vc_num) /* nothing to do */
rc = 0;
@@ -4735,7 +4734,8 @@ int con_font_op(struct vc_data *vc, struct console_font_op *op)
case KD_FONT_OP_SET_DEFAULT:
return con_font_default(vc, op);
case KD_FONT_OP_COPY:
- return con_font_copy(vc, op);
+ /* uses op->height as a console index */
+ return con_font_copy(vc, op->height);
}
return -ENOSYS;
}
diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
index cef437817b0d..cb5b5705ea71 100644
--- a/drivers/video/fbdev/core/fbcon.c
+++ b/drivers/video/fbdev/core/fbcon.c
@@ -2451,7 +2451,7 @@ static int fbcon_do_set_font(struct vc_data *vc, int w, int h,
return 0;
}
-static int fbcon_copy_font(struct vc_data *vc, int con)
+static int fbcon_copy_font(struct vc_data *vc, unsigned int con)
{
struct fbcon_display *od = &fb_display[con];
struct console_font *f = &vc->vc_font;
diff --git a/include/linux/console.h b/include/linux/console.h
index 4b1e26c4cb42..34855d3f2afd 100644
--- a/include/linux/console.h
+++ b/include/linux/console.h
@@ -62,7 +62,7 @@ struct consw {
int (*con_font_get)(struct vc_data *vc, struct console_font *font);
int (*con_font_default)(struct vc_data *vc,
struct console_font *font, char *name);
- int (*con_font_copy)(struct vc_data *vc, int con);
+ int (*con_font_copy)(struct vc_data *vc, unsigned int con);
int (*con_resize)(struct vc_data *vc, unsigned int width,
unsigned int height, unsigned int user);
void (*con_set_palette)(struct vc_data *vc,
--
2.25.1
On 02. 11. 20, 10:36, Peilin Ye wrote:
> `struct console_font` is a UAPI structure, thus ideally should not be
> used for kernel internal abstraction. Remove some dummy .con_font_set,
> .con_font_default and .con_font_copy `struct consw` callback
> implementations, to make it cleaner.
ESEMANTIC_ERROR.
1) What do you refer to with the last "it"?
2) What's the purpose of mentioning struct console_font at all?
3) Could you clarify whether you checked it is safe to remove the hooks?
4) All the hooks now return ENOSYS for both consoles (and not 0). Is
this intentional?
I know answers to the first 3 questions, but you need to elaborate a bit
in the commit log to connect those sentences. Esp. for people not
dealing with the code on a daily basis. Ad 4) I am not sure.
> Suggested-by: Daniel Vetter <[email protected]>
> Signed-off-by: Peilin Ye <[email protected]>
> ---
> Change in v2:
> - [v2 2/2] no longer Cc: stable, so do not Cc: stable
>
> Context: https://lore.kernel.org/lkml/CAKMK7uFY2zv0adjKJ_ORVFT7Zzwn075MaU0rEU7_FuqENLR=UA@mail.gmail.com/
>
> drivers/usb/misc/sisusbvga/sisusb_con.c | 21 ---------------------
> drivers/video/console/dummycon.c | 20 --------------------
> 2 files changed, 41 deletions(-)
>
> diff --git a/drivers/usb/misc/sisusbvga/sisusb_con.c b/drivers/usb/misc/sisusbvga/sisusb_con.c
> index c63e545fb105..dfa0d5ce6012 100644
> --- a/drivers/usb/misc/sisusbvga/sisusb_con.c
> +++ b/drivers/usb/misc/sisusbvga/sisusb_con.c
> @@ -1345,24 +1345,6 @@ static int sisusbdummycon_blank(struct vc_data *vc, int blank, int mode_switch)
> return 0;
> }
>
> -static int sisusbdummycon_font_set(struct vc_data *vc,
> - struct console_font *font,
> - unsigned int flags)
> -{
> - return 0;
> -}
> -
> -static int sisusbdummycon_font_default(struct vc_data *vc,
> - struct console_font *font, char *name)
> -{
> - return 0;
> -}
> -
> -static int sisusbdummycon_font_copy(struct vc_data *vc, int con)
> -{
> - return 0;
> -}
> -
> static const struct consw sisusb_dummy_con = {
> .owner = THIS_MODULE,
> .con_startup = sisusbdummycon_startup,
> @@ -1375,9 +1357,6 @@ static const struct consw sisusb_dummy_con = {
> .con_scroll = sisusbdummycon_scroll,
> .con_switch = sisusbdummycon_switch,
> .con_blank = sisusbdummycon_blank,
> - .con_font_set = sisusbdummycon_font_set,
> - .con_font_default = sisusbdummycon_font_default,
> - .con_font_copy = sisusbdummycon_font_copy,
> };
>
> int
> diff --git a/drivers/video/console/dummycon.c b/drivers/video/console/dummycon.c
> index 2a0d0bda7faa..f1711b2f9ff0 100644
> --- a/drivers/video/console/dummycon.c
> +++ b/drivers/video/console/dummycon.c
> @@ -124,23 +124,6 @@ static int dummycon_switch(struct vc_data *vc)
> return 0;
> }
>
> -static int dummycon_font_set(struct vc_data *vc, struct console_font *font,
> - unsigned int flags)
> -{
> - return 0;
> -}
> -
> -static int dummycon_font_default(struct vc_data *vc,
> - struct console_font *font, char *name)
> -{
> - return 0;
> -}
> -
> -static int dummycon_font_copy(struct vc_data *vc, int con)
> -{
> - return 0;
> -}
> -
> /*
> * The console `switch' structure for the dummy console
> *
> @@ -159,8 +142,5 @@ const struct consw dummy_con = {
> .con_scroll = dummycon_scroll,
> .con_switch = dummycon_switch,
> .con_blank = dummycon_blank,
> - .con_font_set = dummycon_font_set,
> - .con_font_default = dummycon_font_default,
> - .con_font_copy = dummycon_font_copy,
> };
> EXPORT_SYMBOL_GPL(dummy_con);
>
--
js
suse labs
On Mon, Nov 02, 2020 at 04:37:55AM -0500, Peilin Ye wrote:
> con_font_op() is passing an entire `struct console_font_op *` to
> con_font_copy(), but con_font_copy() only uses `op->height`. Additionally,
> con_font_copy() is silently assigning the unsigned `op->height` to the
> signed `con`, then pass it to fbcon_copy_font().
>
> Let con_font_copy() and fbcon_copy_font() pass an unsigned int directly.
> Also, add a comment in con_font_op() for less confusion, since ideally
> `op->height` should not be used as a console index, as the field name
> suggests.
>
> This patch depends on patch "console: Remove dummy con_font_op() callback
> implementations".
>
> Suggested-by: Daniel Vetter <[email protected]>
> Signed-off-by: Peilin Ye <[email protected]>
> ---
> con_font_set(), con_font_get() and con_font_default() also pass an entire
> `console_font_op`.
>
> con_font_get() and con_font_default() actually update the structure (later
> copied to userspace), so let them be.
>
> con_font_set() does not update the structure, but it uses all fields of it
> except `op`. Avoiding passing `console_font_op` to con_font_set() will
> thus make its signature pretty long (6 parameters).
>
> Changes in v2:
> - Remove redundant `con < 0` check in con_font_copy() (kernel test robot
> <[email protected]>)
> - Remove unnecessary range check in fbcon_copy_font(). con_font_copy()
> calls vc_cons_allocated(), which does the check
> - Do not Cc: stable
> - Rewrite the title and commit message accordingly
>
> drivers/tty/vt/vt.c | 8 ++++----
> drivers/video/fbdev/core/fbcon.c | 2 +-
> include/linux/console.h | 2 +-
> 3 files changed, 6 insertions(+), 6 deletions(-)
I'm not sure switching from int to unsigned just here makes much sense.
All the console code is still using int con to index all the various
arrays (I just checked fbcon.c code), and using int to index arrays is
pretty standard. As long as we have the con < 0 check to catch evil
userspace.
There's still the switch from op to int for con_font_copy, but I think
that's better done as part of the larger cleanup we already discussed. And
then maybe also include patch 1 from this series in that rework.
-Daniel
>
> diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
> index 9506a76f3ab6..27821ef97b13 100644
> --- a/drivers/tty/vt/vt.c
> +++ b/drivers/tty/vt/vt.c
> @@ -4704,9 +4704,8 @@ static int con_font_default(struct vc_data *vc, struct console_font_op *op)
> return rc;
> }
>
> -static int con_font_copy(struct vc_data *vc, struct console_font_op *op)
> +static int con_font_copy(struct vc_data *vc, unsigned int con)
> {
> - int con = op->height;
> int rc;
>
>
> @@ -4715,7 +4714,7 @@ static int con_font_copy(struct vc_data *vc, struct console_font_op *op)
> rc = -EINVAL;
> else if (!vc->vc_sw->con_font_copy)
> rc = -ENOSYS;
> - else if (con < 0 || !vc_cons_allocated(con))
> + else if (!vc_cons_allocated(con))
> rc = -ENOTTY;
> else if (con == vc->vc_num) /* nothing to do */
> rc = 0;
> @@ -4735,7 +4734,8 @@ int con_font_op(struct vc_data *vc, struct console_font_op *op)
> case KD_FONT_OP_SET_DEFAULT:
> return con_font_default(vc, op);
> case KD_FONT_OP_COPY:
> - return con_font_copy(vc, op);
> + /* uses op->height as a console index */
> + return con_font_copy(vc, op->height);
> }
> return -ENOSYS;
> }
> diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
> index cef437817b0d..cb5b5705ea71 100644
> --- a/drivers/video/fbdev/core/fbcon.c
> +++ b/drivers/video/fbdev/core/fbcon.c
> @@ -2451,7 +2451,7 @@ static int fbcon_do_set_font(struct vc_data *vc, int w, int h,
> return 0;
> }
>
> -static int fbcon_copy_font(struct vc_data *vc, int con)
> +static int fbcon_copy_font(struct vc_data *vc, unsigned int con)
> {
> struct fbcon_display *od = &fb_display[con];
> struct console_font *f = &vc->vc_font;
> diff --git a/include/linux/console.h b/include/linux/console.h
> index 4b1e26c4cb42..34855d3f2afd 100644
> --- a/include/linux/console.h
> +++ b/include/linux/console.h
> @@ -62,7 +62,7 @@ struct consw {
> int (*con_font_get)(struct vc_data *vc, struct console_font *font);
> int (*con_font_default)(struct vc_data *vc,
> struct console_font *font, char *name);
> - int (*con_font_copy)(struct vc_data *vc, int con);
> + int (*con_font_copy)(struct vc_data *vc, unsigned int con);
> int (*con_resize)(struct vc_data *vc, unsigned int width,
> unsigned int height, unsigned int user);
> void (*con_set_palette)(struct vc_data *vc,
> --
> 2.25.1
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
On Mon, Nov 02, 2020 at 10:47:55AM +0100, Jiri Slaby wrote:
> On 02. 11. 20, 10:36, Peilin Ye wrote:
> > `struct console_font` is a UAPI structure, thus ideally should not be
> > used for kernel internal abstraction. Remove some dummy .con_font_set,
> > .con_font_default and .con_font_copy `struct consw` callback
> > implementations, to make it cleaner.
>
> ESEMANTIC_ERROR.
>
> 1) What do you refer to with the last "it"?
>
> 2) What's the purpose of mentioning struct console_font at all?
>
> 3) Could you clarify whether you checked it is safe to remove the hooks?
>
> 4) All the hooks now return ENOSYS for both consoles (and not 0). Is this
> intentional?
>
> I know answers to the first 3 questions, but you need to elaborate a bit in
> the commit log to connect those sentences. Esp. for people not dealing with
> the code on a daily basis. Ad 4) I am not sure.
Yup the behaviour change from 4) needs to be called out. I think this
should then also be done as part of the large patch series to remove the
dummy functions from all console drivers.
I don't expect the errno change to cause trouble, and it's the more honest
errno - changing fonts not supported is the truth. But if it is, we can
patch that up appropriately when we get a regression report. That's kinda
unavoidable with old crufty uapi like this one here.
Also a bikeshed: Additional information like the patch changelog or
reasons why you do something is imo best to include in the commit message
itself. It ends up looking a bit less tidy sometimes, but often there's
crucial information in these parts that was accidentally left out from the
commit message.
Thanks, Daniel
>
> > Suggested-by: Daniel Vetter <[email protected]>
> > Signed-off-by: Peilin Ye <[email protected]>
> > ---
> > Change in v2:
> > - [v2 2/2] no longer Cc: stable, so do not Cc: stable
> >
> > Context: https://lore.kernel.org/lkml/CAKMK7uFY2zv0adjKJ_ORVFT7Zzwn075MaU0rEU7_FuqENLR=UA@mail.gmail.com/
> >
> > drivers/usb/misc/sisusbvga/sisusb_con.c | 21 ---------------------
> > drivers/video/console/dummycon.c | 20 --------------------
> > 2 files changed, 41 deletions(-)
> >
> > diff --git a/drivers/usb/misc/sisusbvga/sisusb_con.c b/drivers/usb/misc/sisusbvga/sisusb_con.c
> > index c63e545fb105..dfa0d5ce6012 100644
> > --- a/drivers/usb/misc/sisusbvga/sisusb_con.c
> > +++ b/drivers/usb/misc/sisusbvga/sisusb_con.c
> > @@ -1345,24 +1345,6 @@ static int sisusbdummycon_blank(struct vc_data *vc, int blank, int mode_switch)
> > return 0;
> > }
> > -static int sisusbdummycon_font_set(struct vc_data *vc,
> > - struct console_font *font,
> > - unsigned int flags)
> > -{
> > - return 0;
> > -}
> > -
> > -static int sisusbdummycon_font_default(struct vc_data *vc,
> > - struct console_font *font, char *name)
> > -{
> > - return 0;
> > -}
> > -
> > -static int sisusbdummycon_font_copy(struct vc_data *vc, int con)
> > -{
> > - return 0;
> > -}
> > -
> > static const struct consw sisusb_dummy_con = {
> > .owner = THIS_MODULE,
> > .con_startup = sisusbdummycon_startup,
> > @@ -1375,9 +1357,6 @@ static const struct consw sisusb_dummy_con = {
> > .con_scroll = sisusbdummycon_scroll,
> > .con_switch = sisusbdummycon_switch,
> > .con_blank = sisusbdummycon_blank,
> > - .con_font_set = sisusbdummycon_font_set,
> > - .con_font_default = sisusbdummycon_font_default,
> > - .con_font_copy = sisusbdummycon_font_copy,
> > };
> > int
> > diff --git a/drivers/video/console/dummycon.c b/drivers/video/console/dummycon.c
> > index 2a0d0bda7faa..f1711b2f9ff0 100644
> > --- a/drivers/video/console/dummycon.c
> > +++ b/drivers/video/console/dummycon.c
> > @@ -124,23 +124,6 @@ static int dummycon_switch(struct vc_data *vc)
> > return 0;
> > }
> > -static int dummycon_font_set(struct vc_data *vc, struct console_font *font,
> > - unsigned int flags)
> > -{
> > - return 0;
> > -}
> > -
> > -static int dummycon_font_default(struct vc_data *vc,
> > - struct console_font *font, char *name)
> > -{
> > - return 0;
> > -}
> > -
> > -static int dummycon_font_copy(struct vc_data *vc, int con)
> > -{
> > - return 0;
> > -}
> > -
> > /*
> > * The console `switch' structure for the dummy console
> > *
> > @@ -159,8 +142,5 @@ const struct consw dummy_con = {
> > .con_scroll = dummycon_scroll,
> > .con_switch = dummycon_switch,
> > .con_blank = dummycon_blank,
> > - .con_font_set = dummycon_font_set,
> > - .con_font_default = dummycon_font_default,
> > - .con_font_copy = dummycon_font_copy,
> > };
> > EXPORT_SYMBOL_GPL(dummy_con);
> >
>
>
> --
> js
> suse labs
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
On Mon, Nov 02, 2020 at 11:13:47AM +0100, Daniel Vetter wrote:
> On Mon, Nov 02, 2020 at 10:47:55AM +0100, Jiri Slaby wrote:
> > On 02. 11. 20, 10:36, Peilin Ye wrote:
> > > `struct console_font` is a UAPI structure, thus ideally should not be
> > > used for kernel internal abstraction. Remove some dummy .con_font_set,
> > > .con_font_default and .con_font_copy `struct consw` callback
> > > implementations, to make it cleaner.
> >
> > ESEMANTIC_ERROR.
> >
> > 1) What do you refer to with the last "it"?
> >
> > 2) What's the purpose of mentioning struct console_font at all?
> >
> > 3) Could you clarify whether you checked it is safe to remove the hooks?
I see. I will try to elaborate in future patches, thank you!
> > 4) All the hooks now return ENOSYS for both consoles (and not 0). Is this
> > intentional?
> >
> > I know answers to the first 3 questions, but you need to elaborate a bit in
> > the commit log to connect those sentences. Esp. for people not dealing with
> > the code on a daily basis. Ad 4) I am not sure.
>
> Yup the behaviour change from 4) needs to be called out. I think this
> should then also be done as part of the large patch series to remove the
> dummy functions from all console drivers.
>
> I don't expect the errno change to cause trouble, and it's the more honest
> errno - changing fonts not supported is the truth. But if it is, we can
> patch that up appropriately when we get a regression report. That's kinda
> unavoidable with old crufty uapi like this one here.
>
> Also a bikeshed: Additional information like the patch changelog or
> reasons why you do something is imo best to include in the commit message
> itself. It ends up looking a bit less tidy sometimes, but often there's
> crucial information in these parts that was accidentally left out from the
> commit message.
Sure, I will try to include sufficient information for one to easily
understand what I'm trying to do with a patch. Thank you,
Peilin
On Mon, Nov 02, 2020 at 11:10:44AM +0100, Daniel Vetter wrote:
> I'm not sure switching from int to unsigned just here makes much sense.
> All the console code is still using int con to index all the various
> arrays (I just checked fbcon.c code), and using int to index arrays is
> pretty standard. As long as we have the con < 0 check to catch evil
> userspace.
>
> There's still the switch from op to int for con_font_copy, but I think
> that's better done as part of the larger cleanup we already discussed. And
> then maybe also include patch 1 from this series in that rework.
I see. I think at the moment there's not much we can do for
con_font_get/set/default(). _get() and _default() use *op, and _set()
uses all except one field of *op. Maybe we can change the type of *op
from console_font_op to font_desc, after cleaning up everything else?
Peilin
On Mon, Nov 2, 2020 at 12:12 PM Peilin Ye <[email protected]> wrote:
>
> On Mon, Nov 02, 2020 at 11:10:44AM +0100, Daniel Vetter wrote:
> > I'm not sure switching from int to unsigned just here makes much sense.
> > All the console code is still using int con to index all the various
> > arrays (I just checked fbcon.c code), and using int to index arrays is
> > pretty standard. As long as we have the con < 0 check to catch evil
> > userspace.
> >
> > There's still the switch from op to int for con_font_copy, but I think
> > that's better done as part of the larger cleanup we already discussed. And
> > then maybe also include patch 1 from this series in that rework.
>
> I see. I think at the moment there's not much we can do for
> con_font_get/set/default(). _get() and _default() use *op, and _set()
> uses all except one field of *op. Maybe we can change the type of *op
> from console_font_op to font_desc, after cleaning up everything else?
Yeah, for these one of the arguments should be the new font_desc, so
that we can remove the op stuff properly. Opening up all the arguments
without the font_desc doesn't make sense imo.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
On Sat, Oct 31, 2020 at 03:24:41AM -0400, Peilin Ye wrote:
> `struct console_font` is a UAPI structure, thus ideally should not be
> used for kernel internal abstraction. Remove some dummy .con_font_set,
> .con_font_default and .con_font_copy `struct consw` callback
> implementations, to make it cleaner.
>
> Patch "fbcon: Prevent global-out-of-bounds read in fbcon_copy_font()"
> depends on this patch, so Cc: stable.
>
> Cc: [email protected]
> Suggested-by: Daniel Vetter <[email protected]>
> Signed-off-by: Peilin Ye <[email protected]>
> ---
> Context: https://lore.kernel.org/lkml/CAKMK7uFY2zv0adjKJ_ORVFT7Zzwn075MaU0rEU7_FuqENLR=UA@mail.gmail.com/
>
> drivers/usb/misc/sisusbvga/sisusb_con.c | 21 ---------------------
> drivers/video/console/dummycon.c | 20 --------------------
> 2 files changed, 41 deletions(-)
Reviewed-by: Greg Kroah-Hartman <[email protected]>
On Fri, Nov 06, 2020 at 11:50:58AM +0100, Greg Kroah-Hartman wrote:
> On Sat, Oct 31, 2020 at 03:24:41AM -0400, Peilin Ye wrote:
> > `struct console_font` is a UAPI structure, thus ideally should not be
> > used for kernel internal abstraction. Remove some dummy .con_font_set,
> > .con_font_default and .con_font_copy `struct consw` callback
> > implementations, to make it cleaner.
> >
> > Patch "fbcon: Prevent global-out-of-bounds read in fbcon_copy_font()"
> > depends on this patch, so Cc: stable.
> >
> > Cc: [email protected]
> > Suggested-by: Daniel Vetter <[email protected]>
> > Signed-off-by: Peilin Ye <[email protected]>
> > ---
> > Context: https://lore.kernel.org/lkml/CAKMK7uFY2zv0adjKJ_ORVFT7Zzwn075MaU0rEU7_FuqENLR=UA@mail.gmail.com/
> >
> > drivers/usb/misc/sisusbvga/sisusb_con.c | 21 ---------------------
> > drivers/video/console/dummycon.c | 20 --------------------
> > 2 files changed, 41 deletions(-)
>
> Reviewed-by: Greg Kroah-Hartman <[email protected]>
Peilin, can you pls resend this together with all the other pending
patches from you? I think that's better than me trying to cherry-pick the
bits we decided to keep from random places.
Greg, ok if I just pull these in through drm-misc-next? It's a pretty bad
hairball anyway and that avoids the tree coordination issues. Only thing
that might get in the way is the vt font_copy removal, but that's in -rc3
so easy to backmerge.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
On Tue, Nov 10, 2020 at 01:49:46PM +0100, Daniel Vetter wrote:
> Peilin, can you pls resend this together with all the other pending
> patches from you? I think that's better than me trying to cherry-pick the
> bits we decided to keep from random places.
Oh, are we doing an -rc3 backmerge soon? At the moment I can base these
patches on neither drm-misc (due to the font_copy removal), nor mainline
(due to the signedness issue in font_desc we've talked about), so I'm
waiting for a backmerge to rebase everything properly. Sorry that I
didn't mention earlier.
> Greg, ok if I just pull these in through drm-misc-next? It's a pretty bad
> hairball anyway and that avoids the tree coordination issues. Only thing
> that might get in the way is the vt font_copy removal, but that's in -rc3
> so easy to backmerge.
I will rebase and send everything (including the font_copy
garbage-collecting) in a v3 series after the backmerge. Thanks,
Peilin Ye
On Tue, Nov 10, 2020 at 2:24 PM Peilin Ye <[email protected]> wrote:
>
> On Tue, Nov 10, 2020 at 01:49:46PM +0100, Daniel Vetter wrote:
> > Peilin, can you pls resend this together with all the other pending
> > patches from you? I think that's better than me trying to cherry-pick the
> > bits we decided to keep from random places.
>
> Oh, are we doing an -rc3 backmerge soon? At the moment I can base these
> patches on neither drm-misc (due to the font_copy removal), nor mainline
> (due to the signedness issue in font_desc we've talked about), so I'm
> waiting for a backmerge to rebase everything properly. Sorry that I
> didn't mention earlier.
linux-next has all the trees, so you can always use that. And yes I'm
pushing the backmerge through, so in a few days at most I can pull in
all your patches. Meanwhile you can base your work of linux-next.
> > Greg, ok if I just pull these in through drm-misc-next? It's a pretty bad
> > hairball anyway and that avoids the tree coordination issues. Only thing
> > that might get in the way is the vt font_copy removal, but that's in -rc3
> > so easy to backmerge.
>
> I will rebase and send everything (including the font_copy
> garbage-collecting) in a v3 series after the backmerge. Thanks,
No need to be blocked on a backmerge, this is only needed for merging
the patches. Development should not be blocked like this.
-Daniel
>
> Peilin Ye
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
On Tue, Nov 10, 2020 at 02:46:20PM +0100, Daniel Vetter wrote:
> On Tue, Nov 10, 2020 at 2:24 PM Peilin Ye <[email protected]> wrote:
> > Oh, are we doing an -rc3 backmerge soon? At the moment I can base these
> > patches on neither drm-misc (due to the font_copy removal), nor mainline
> > (due to the signedness issue in font_desc we've talked about), so I'm
> > waiting for a backmerge to rebase everything properly. Sorry that I
> > didn't mention earlier.
>
> linux-next has all the trees, so you can always use that. And yes I'm
> pushing the backmerge through, so in a few days at most I can pull in
> all your patches. Meanwhile you can base your work of linux-next.
>
> > > Greg, ok if I just pull these in through drm-misc-next? It's a pretty bad
> > > hairball anyway and that avoids the tree coordination issues. Only thing
> > > that might get in the way is the vt font_copy removal, but that's in -rc3
> > > so easy to backmerge.
> >
> > I will rebase and send everything (including the font_copy
> > garbage-collecting) in a v3 series after the backmerge. Thanks,
>
> No need to be blocked on a backmerge, this is only needed for merging
> the patches. Development should not be blocked like this.
I see. Thanks!
Peilin Ye
On Tue, Nov 10, 2020 at 01:49:46PM +0100, Daniel Vetter wrote:
> On Fri, Nov 06, 2020 at 11:50:58AM +0100, Greg Kroah-Hartman wrote:
> > On Sat, Oct 31, 2020 at 03:24:41AM -0400, Peilin Ye wrote:
> > > `struct console_font` is a UAPI structure, thus ideally should not be
> > > used for kernel internal abstraction. Remove some dummy .con_font_set,
> > > .con_font_default and .con_font_copy `struct consw` callback
> > > implementations, to make it cleaner.
> > >
> > > Patch "fbcon: Prevent global-out-of-bounds read in fbcon_copy_font()"
> > > depends on this patch, so Cc: stable.
> > >
> > > Cc: [email protected]
> > > Suggested-by: Daniel Vetter <[email protected]>
> > > Signed-off-by: Peilin Ye <[email protected]>
> > > ---
> > > Context: https://lore.kernel.org/lkml/CAKMK7uFY2zv0adjKJ_ORVFT7Zzwn075MaU0rEU7_FuqENLR=UA@mail.gmail.com/
> > >
> > > drivers/usb/misc/sisusbvga/sisusb_con.c | 21 ---------------------
> > > drivers/video/console/dummycon.c | 20 --------------------
> > > 2 files changed, 41 deletions(-)
> >
> > Reviewed-by: Greg Kroah-Hartman <[email protected]>
>
> Peilin, can you pls resend this together with all the other pending
> patches from you? I think that's better than me trying to cherry-pick the
> bits we decided to keep from random places.
>
> Greg, ok if I just pull these in through drm-misc-next? It's a pretty bad
> hairball anyway and that avoids the tree coordination issues. Only thing
> that might get in the way is the vt font_copy removal, but that's in -rc3
> so easy to backmerge.
Yes please take them all!
thanks,
greg k-h