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 <[email protected]>
Signed-off-by: George Kennedy <[email protected]>
---
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
On Mon, Feb 27, 2023 at 12:22 PM George Kennedy
<[email protected]> 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
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/[email protected]/
> Fixes: 226fae124b2d ("vc_screen: move load of struct vc_data pointer in vcs_read() to avoid UAF")
> Suggested-by: Jiri Slaby <[email protected]>
> Signed-off-by: George Kennedy <[email protected]>
> ---
> 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
>
On Mon, Feb 27, 2023 at 5:46 PM <[email protected]> wrote:
>
> This still seems to be broken for me.
Looks that way.
> I still need the patch from
>
> https://lore.kernel.org/lkml/[email protected]/
.. 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
On Mon, Feb 27, 2023 at 05:58:12PM -0800, Linus Torvalds wrote:
> On Mon, Feb 27, 2023 at 5:46 PM <[email protected]> wrote:
> >
> > This still seems to be broken for me.
>
> Looks that way.
>
> > I still need the patch from
> >
> > https://lore.kernel.org/lkml/[email protected]/
>
> .. 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 <[email protected]>
(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
On Mon, Feb 27, 2023 at 6:18 PM Thomas Weißschuh <[email protected]> wrote:
>
> Tested-By: Thomas Weißschuh <[email protected]>
>
> (Or feel free to use my patch from above and fixup the goto/break line)
Done.
Linus