* [PATCH v2] vc_screen: modify vcs_size() handling in vcs_read()
@ 2023-02-27 20:21 George Kennedy
2023-02-28 0:12 ` Linus Torvalds
2023-02-28 1:45 ` linux
0 siblings, 2 replies; 6+ messages in thread
From: George Kennedy @ 2023-02-27 20:21 UTC (permalink / raw)
To: jirislaby, gregkh, torvalds
Cc: george.kennedy, sfr, akpm, linux-kernel, linux-serial, linux,
regressions
Restore the vcs_size() handling in vcs_read() to what
it had been in previous version.
Fixes: 226fae124b2d ("vc_screen: move load of struct vc_data pointer in vcs_read() to avoid UAF")
Suggested-by: Jiri Slaby <jirislaby@kernel.org>
Signed-off-by: George Kennedy <george.kennedy@oracle.com>
---
v2: added Fixes
drivers/tty/vt/vc_screen.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/tty/vt/vc_screen.c b/drivers/tty/vt/vc_screen.c
index f566eb1839dc..c0a2273bb998 100644
--- a/drivers/tty/vt/vc_screen.c
+++ b/drivers/tty/vt/vc_screen.c
@@ -414,10 +414,8 @@ vcs_read(struct file *file, char __user *buf, size_t count, loff_t *ppos)
*/
size = vcs_size(vc, attr, uni_mode);
if (size < 0) {
- if (read)
- break;
ret = size;
- goto unlock_out;
+ break;
}
if (pos >= size)
break;
--
2.31.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] vc_screen: modify vcs_size() handling in vcs_read()
2023-02-27 20:21 [PATCH v2] vc_screen: modify vcs_size() handling in vcs_read() George Kennedy
@ 2023-02-28 0:12 ` Linus Torvalds
2023-02-28 1:45 ` linux
1 sibling, 0 replies; 6+ messages in thread
From: Linus Torvalds @ 2023-02-28 0:12 UTC (permalink / raw)
To: George Kennedy
Cc: jirislaby, gregkh, sfr, akpm, linux-kernel, linux-serial, linux,
regressions
On Mon, Feb 27, 2023 at 12:22 PM George Kennedy
<george.kennedy@oracle.com> wrote:
>
> Restore the vcs_size() handling in vcs_read() to what
> it had been in previous version.
I took this one directly since I'd been involved in the discussion the
whole time, so we can just close this issue.
Linus
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] vc_screen: modify vcs_size() handling in vcs_read()
2023-02-27 20:21 [PATCH v2] vc_screen: modify vcs_size() handling in vcs_read() George Kennedy
2023-02-28 0:12 ` Linus Torvalds
@ 2023-02-28 1:45 ` linux
2023-02-28 1:58 ` Linus Torvalds
1 sibling, 1 reply; 6+ messages in thread
From: linux @ 2023-02-28 1:45 UTC (permalink / raw)
To: George Kennedy
Cc: jirislaby, gregkh, torvalds, sfr, akpm, linux-kernel,
linux-serial, regressions
Hi,
On Mon, Feb 27, 2023 at 03:21:41PM -0500, George Kennedy wrote:
> Restore the vcs_size() handling in vcs_read() to what
> it had been in previous version.
This still seems to be broken for me.
Testcase:
# cat /dev/vcsa1
[.. data, looks complete ..]
cat: /dev/vcsa1: No such device or address
"ret" is still unconditionally overwritten with -ENXIO at the beginning
of the loop.
And when we break the loop because everything has been read in
`if (pos >= size)` then this error is returned to userspace instead of
just `0`.
I still need the patch from
https://lore.kernel.org/lkml/20230220064612.1783-1-linux@weissschuh.net/
> Fixes: 226fae124b2d ("vc_screen: move load of struct vc_data pointer in vcs_read() to avoid UAF")
> Suggested-by: Jiri Slaby <jirislaby@kernel.org>
> Signed-off-by: George Kennedy <george.kennedy@oracle.com>
> ---
> v2: added Fixes
>
> drivers/tty/vt/vc_screen.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/tty/vt/vc_screen.c b/drivers/tty/vt/vc_screen.c
> index f566eb1839dc..c0a2273bb998 100644
> --- a/drivers/tty/vt/vc_screen.c
> +++ b/drivers/tty/vt/vc_screen.c
> @@ -414,10 +414,8 @@ vcs_read(struct file *file, char __user *buf, size_t count, loff_t *ppos)
> */
> size = vcs_size(vc, attr, uni_mode);
> if (size < 0) {
> - if (read)
> - break;
> ret = size;
> - goto unlock_out;
> + break;
> }
> if (pos >= size)
> break;
> --
> 2.31.1
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] vc_screen: modify vcs_size() handling in vcs_read()
2023-02-28 1:45 ` linux
@ 2023-02-28 1:58 ` Linus Torvalds
2023-02-28 2:18 ` Thomas Weißschuh
0 siblings, 1 reply; 6+ messages in thread
From: Linus Torvalds @ 2023-02-28 1:58 UTC (permalink / raw)
To: linux
Cc: George Kennedy, jirislaby, gregkh, sfr, akpm, linux-kernel,
linux-serial, regressions
On Mon, Feb 27, 2023 at 5:46 PM <linux@weissschuh.net> wrote:
>
> This still seems to be broken for me.
Looks that way.
> I still need the patch from
>
> https://lore.kernel.org/lkml/20230220064612.1783-1-linux@weissschuh.net/
.. and that has the same problem with "what if the error happens
during an iteration that wasn't the first, and we already succeeded
partially".
The "goto unlock_out" is bogus, since it jumps over all the "update
pos and check if we read something".
It was the correct thing to do *above* the loop, but not inside the loop.
IOW, I think the proper patch is to also turn the "goto unlock_out"
into a "break". Mind testing something like this (whitespace-damaged,
but you get the idea):
--- a/drivers/tty/vt/vc_screen.c
+++ b/drivers/tty/vt/vc_screen.c
@@ -403,10 +403,11 @@ vcs_read(struct file *file, char __user
*buf, size_t count, loff_t *ppos)
unsigned int this_round, skip = 0;
int size;
- ret = -ENXIO;
vc = vcs_vc(inode, &viewed);
- if (!vc)
- goto unlock_out;
+ if (!vc) {
+ ret = -ENXIO;
+ break;
+ }
/* Check whether we are above size each round,
* as copy_to_user at the end of this loop
which hopefully really fixes this (at least I don't see any other
"goto unlock_out" cases).
Linus
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] vc_screen: modify vcs_size() handling in vcs_read()
2023-02-28 1:58 ` Linus Torvalds
@ 2023-02-28 2:18 ` Thomas Weißschuh
2023-02-28 2:40 ` Linus Torvalds
0 siblings, 1 reply; 6+ messages in thread
From: Thomas Weißschuh @ 2023-02-28 2:18 UTC (permalink / raw)
To: Linus Torvalds
Cc: George Kennedy, jirislaby, gregkh, sfr, akpm, linux-kernel,
linux-serial, regressions
On Mon, Feb 27, 2023 at 05:58:12PM -0800, Linus Torvalds wrote:
> On Mon, Feb 27, 2023 at 5:46 PM <linux@weissschuh.net> wrote:
> >
> > This still seems to be broken for me.
>
> Looks that way.
>
> > I still need the patch from
> >
> > https://lore.kernel.org/lkml/20230220064612.1783-1-linux@weissschuh.net/
>
> .. and that has the same problem with "what if the error happens
> during an iteration that wasn't the first, and we already succeeded
> partially".
Indeed.
> The "goto unlock_out" is bogus, since it jumps over all the "update
> pos and check if we read something".
>
> It was the correct thing to do *above* the loop, but not inside the loop.
>
> IOW, I think the proper patch is to also turn the "goto unlock_out"
> into a "break". Mind testing something like this (whitespace-damaged,
> but you get the idea):
Makes sense and seems to work correctly.
Tested-By: Thomas Weißschuh <linux@weissschuh.net>
(Or feel free to use my patch from above and fixup the goto/break line)
>
> --- a/drivers/tty/vt/vc_screen.c
> +++ b/drivers/tty/vt/vc_screen.c
> @@ -403,10 +403,11 @@ vcs_read(struct file *file, char __user
> *buf, size_t count, loff_t *ppos)
> unsigned int this_round, skip = 0;
> int size;
>
> - ret = -ENXIO;
> vc = vcs_vc(inode, &viewed);
> - if (!vc)
> - goto unlock_out;
> + if (!vc) {
> + ret = -ENXIO;
> + break;
> + }
>
> /* Check whether we are above size each round,
> * as copy_to_user at the end of this loop
>
> which hopefully really fixes this (at least I don't see any other
> "goto unlock_out" cases).
>
> Linus
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] vc_screen: modify vcs_size() handling in vcs_read()
2023-02-28 2:18 ` Thomas Weißschuh
@ 2023-02-28 2:40 ` Linus Torvalds
0 siblings, 0 replies; 6+ messages in thread
From: Linus Torvalds @ 2023-02-28 2:40 UTC (permalink / raw)
To: Thomas Weißschuh
Cc: George Kennedy, jirislaby, gregkh, sfr, akpm, linux-kernel,
linux-serial, regressions
On Mon, Feb 27, 2023 at 6:18 PM Thomas Weißschuh <linux@weissschuh.net> wrote:
>
> Tested-By: Thomas Weißschuh <linux@weissschuh.net>
>
> (Or feel free to use my patch from above and fixup the goto/break line)
Done.
Linus
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-02-28 2:40 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-02-27 20:21 [PATCH v2] vc_screen: modify vcs_size() handling in vcs_read() George Kennedy
2023-02-28 0:12 ` Linus Torvalds
2023-02-28 1:45 ` linux
2023-02-28 1:58 ` Linus Torvalds
2023-02-28 2:18 ` Thomas Weißschuh
2023-02-28 2:40 ` Linus Torvalds
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).