On 2/20/2023 11:34 AM, Thomas Weißschuh wrote:
+Cc people who were involved in the original thread.
On Mon, Feb 20, 2023 at 12:48:59PM +0100, Jiri Slaby wrote:
On 20. 02. 23, 7:46, linux@xxxxxxxxxxxxxx wrote:
From: Thomas Weißschuh <linux@xxxxxxxxxxxxxx>
Commit 226fae124b2d
("vc_screen: move load of struct vc_data pointer in vcs_read() to avoid UAF")
moved the call to vcs_vc() into the loop.
While doing this it also moved the unconditional assignment of
"ret = -ENXIO".
This unconditional assignment was valid outside the loop but within it
it clobbers the actual value of ret.
To avoid this only assign "ret = -ENXIO" when actually needed.
Not sure -- I cannot find it -- but hasn't George fixed this yet?
Indeed there was a proposed fix at
https://lore.kernel.org/lkml/1675704844-17228-1-git-send-email-george.kennedy@xxxxxxxxxx/
Linus had some suggestions so it was not applied as is.
I'm not sure what the current state is.
George, do you have something in the pipeline?
Yes, that is in the pipeline:
https://lore.kernel.org/lkml/1675774098-17722-1-git-send-email-george.kennedy@xxxxxxxxxx/
Linus suggested the fix, which was tested and submitted.
Jiri commented on the patch, which I believe was directed at Linus as he
suggested the fix.
George
I also tested the patch proposed by Linus as attachment and that works.
(The small inline patch snippet doesn't)
Reported-by: Storm Dragon <stormdragon2976@xxxxxxxxx>
Link: https://lore.kernel.org/lkml/Y%2FKS6vdql2pIsCiI@xxxxxxxxxxx/
Fixes: 226fae124b2d ("vc_screen: move load of struct vc_data pointer in vcs_read() to avoid UAF")
Signed-off-by: Thomas Weißschuh <linux@xxxxxxxxxxxxxx>
---
@Storm Could you validate this patch?
---
drivers/tty/vt/vc_screen.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/tty/vt/vc_screen.c b/drivers/tty/vt/vc_screen.c
index f566eb1839dc..2ef519a40a87 100644
--- 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)
+ if (!vc) {
+ ret = -ENXIO;
goto unlock_out;
+ }
/* Check whether we are above size each round,
* as copy_to_user at the end of this loop
base-commit: c9c3395d5e3dcc6daee66c6908354d47bf98cb0c
--
js
suse labs
