| Message ID | 1677529301-19530-1-git-send-email-george.kennedy@oracle.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] vc_screen: modify vcs_size() handling in vcs_read() | expand |
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
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 >
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
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
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
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;
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(-)