# \[PATCH 0/8\] xen: harden frontends against malicious backends - Subsystems: HYPERVISOR VIRTUAL CONSOLE DRIVER, BLOCK LAYER, NETWORKING DRIVERS, THE REST, TTY LAYER AND SERIAL DRIVERS, XEN BLOCK SUBSYSTEM, XEN HYPERVISOR INTERFACE - Maintainers: Jens Axboe, Andrew Lunn, "David S. Miller", Eric Dumazet, Jakub Kicinski, Paolo Abeni, Linus Torvalds, Greg Kroah-Hartman, Jiri Slaby, Roger Pau Monné, Juergen Gross, Stefano Stabellini - Series: 8 patches touching 4 files, +371/-228 - List: linuxppc-dev - 7 messages, 4 authors - Page: https://ratatoskr.run/linuxppc-dev/2021/05/1376540/t ## \[PATCH 0/8\] xen: harden frontends against malicious backends - From: Juergen Gross \ - Date: 2021-05-13 10:03:36 - Message: https://ratatoskr.run/linuxppc-dev/2021/05/1376540.md ```text Xen backends of para-virtualized devices can live in dom0 kernel, dom0 user land, or in a driver domain. This means that a backend might reside in a less trusted environment than the Xen core components, so a backend should not be able to do harm to a Xen guest (it can still mess up I/O data, but it shouldn't be able to e.g. crash a guest by other means or cause a privilege escalation in the guest). Unfortunately many frontends in the Linux kernel are fully trusting their respective backends. This series is starting to fix the most important frontends: console, disk and network. It was discussed to handle this as a security problem, but the topic was discussed in public before, so it isn't a real secret. Juergen Gross (8): xen: sync include/xen/interface/io/ring.h with Xen's newest version xen/blkfront: read response from backend only once xen/blkfront: don't take local copy of a request from the ring page xen/blkfront: don't trust the backend response data blindly xen/netfront: read response from backend only once xen/netfront: don't read data from request on the ring page xen/netfront: don't trust the backend response data blindly xen/hvc: replace BUG_ON() with negative return value drivers/block/xen-blkfront.c | 118 +++++++++----- drivers/net/xen-netfront.c | 184 ++++++++++++++------- drivers/tty/hvc/hvc_xen.c | 15 +- include/xen/interface/io/ring.h | 278 ++++++++++++++++++-------------- 4 files changed, 369 insertions(+), 226 deletions(-) -- 2.26.2 ``` ## \[PATCH 8/8\] xen/hvc: replace BUG\_ON() with negative return value - From: Juergen Gross \ - Date: 2021-05-13 10:06:05 - Message: https://ratatoskr.run/linuxppc-dev/2021/05/1376548.md ```text Xen frontends shouldn't BUG() in case of illegal data received from their backends. So replace the BUG_ON()s when reading illegal data from the ring page with negative return values. Signed-off-by: Juergen Gross --- drivers/tty/hvc/hvc_xen.c | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/drivers/tty/hvc/hvc_xen.c b/drivers/tty/hvc/hvc_xen.c index 92c9a476defc..30d7ffb1e04c 100644 --- a/drivers/tty/hvc/hvc_xen.c +++ b/drivers/tty/hvc/hvc_xen.c @@ -86,6 +86,11 @@ static int __write_console(struct xencons_info *xencons, cons = intf->out_cons; prod = intf->out_prod; mb(); /* update queue values before going on */ + + if (WARN_ONCE((prod - cons) > sizeof(intf->out), + "Illegal ring page indices")) + return -EINVAL; + BUG_ON((prod - cons) > sizeof(intf->out)); while ((sent < len) && ((prod - cons) < sizeof(intf->out))) @@ -114,7 +119,10 @@ static int domU_write_console(uint32_t vtermno, const char *data, int len) */ while (len) { int sent = __write_console(cons, data, len); - + + if (sent < 0) + return sent; + data += sent; len -= sent; @@ -138,7 +146,10 @@ static int domU_read_console(uint32_t vtermno, char *buf, int len) cons = intf->in_cons; prod = intf->in_prod; mb(); /* get pointers before reading ring */ - BUG_ON((prod - cons) > sizeof(intf->in)); + + if (WARN_ONCE((prod - cons) > sizeof(intf->in), + "Illegal ring page indices")) + return -EINVAL; while (cons != prod && recv < len) buf[recv++] = intf->in[MASK_XENCONS_IDX(cons++, intf->in)]; -- 2.26.2 ``` ## Re: \[PATCH 8/8\] xen/hvc: replace BUG\_ON() with negative return value - From: Christophe Leroy \ - Date: 2021-05-13 10:16:42 - Message: https://ratatoskr.run/linuxppc-dev/2021/05/1376555.md ```text Le 13/05/2021 à 12:03, Juergen Gross a écrit : > Xen frontends shouldn't BUG() in case of illegal data received from > their backends. So replace the BUG_ON()s when reading illegal data from > the ring page with negative return values. > > Signed-off-by: Juergen Gross > --- > drivers/tty/hvc/hvc_xen.c | 15 +++++++++++++-- > 1 file changed, 13 insertions(+), 2 deletions(-) > > diff --git a/drivers/tty/hvc/hvc_xen.c b/drivers/tty/hvc/hvc_xen.c > index 92c9a476defc..30d7ffb1e04c 100644 > --- a/drivers/tty/hvc/hvc_xen.c > +++ b/drivers/tty/hvc/hvc_xen.c > @@ -86,6 +86,11 @@ static int __write_console(struct xencons_info *xencons, > cons = intf->out_cons; > prod = intf->out_prod; > mb(); /* update queue values before going on */ > + > + if (WARN_ONCE((prod - cons) > sizeof(intf->out), > + "Illegal ring page indices")) > + return -EINVAL; > + > BUG_ON((prod - cons) > sizeof(intf->out)); Why keep the BUG_ON() ? > > while ((sent < len) && ((prod - cons) < sizeof(intf->out))) > @@ -114,7 +119,10 @@ static int domU_write_console(uint32_t vtermno, const char *data, int len) > */ > while (len) { > int sent = __write_console(cons, data, len); > - > + > + if (sent < 0) > + return sent; > + > data += sent; > len -= sent; > > @@ -138,7 +146,10 @@ static int domU_read_console(uint32_t vtermno, char *buf, int len) > cons = intf->in_cons; > prod = intf->in_prod; > mb(); /* get pointers before reading ring */ > - BUG_ON((prod - cons) > sizeof(intf->in)); > + > + if (WARN_ONCE((prod - cons) > sizeof(intf->in), > + "Illegal ring page indices")) > + return -EINVAL; > > while (cons != prod && recv < len) > buf[recv++] = intf->in[MASK_XENCONS_IDX(cons++, intf->in)]; > ``` ## Re: \[PATCH 8/8\] xen/hvc: replace BUG\_ON() with negative return value - From: Juergen Gross \ - Date: 2021-05-13 10:21:12 - Message: https://ratatoskr.run/linuxppc-dev/2021/05/1376559.md ```text On 13.05.21 12:16, Christophe Leroy wrote: > > > Le 13/05/2021 à 12:03, Juergen Gross a écrit : >> Xen frontends shouldn't BUG() in case of illegal data received from >> their backends. So replace the BUG_ON()s when reading illegal data from >> the ring page with negative return values. >> >> Signed-off-by: Juergen Gross >> --- >>   drivers/tty/hvc/hvc_xen.c | 15 +++++++++++++-- >>   1 file changed, 13 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/tty/hvc/hvc_xen.c b/drivers/tty/hvc/hvc_xen.c >> index 92c9a476defc..30d7ffb1e04c 100644 >> --- a/drivers/tty/hvc/hvc_xen.c >> +++ b/drivers/tty/hvc/hvc_xen.c >> @@ -86,6 +86,11 @@ static int __write_console(struct xencons_info *xencons, >>       cons = intf->out_cons; >>       prod = intf->out_prod; >>       mb();            /* update queue values before going on */ >> + >> +    if (WARN_ONCE((prod - cons) > sizeof(intf->out), >> +              "Illegal ring page indices")) >> +        return -EINVAL; >> + >>       BUG_ON((prod - cons) > sizeof(intf->out)); > > Why keep the BUG_ON() ? Oh, failed to delete it. Thanks for noticing. Juergen ``` ## Re: \[PATCH 8/8\] xen/hvc: replace BUG\_ON() with negative return value - From: Greg Kroah-Hartman \ - Date: 2021-05-13 10:25:52 - Message: https://ratatoskr.run/linuxppc-dev/2021/05/1376561.md ```text On Thu, May 13, 2021 at 12:03:02PM +0200, Juergen Gross wrote: > Xen frontends shouldn't BUG() in case of illegal data received from > their backends. So replace the BUG_ON()s when reading illegal data from > the ring page with negative return values. > > Signed-off-by: Juergen Gross > --- > drivers/tty/hvc/hvc_xen.c | 15 +++++++++++++-- > 1 file changed, 13 insertions(+), 2 deletions(-) > > diff --git a/drivers/tty/hvc/hvc_xen.c b/drivers/tty/hvc/hvc_xen.c > index 92c9a476defc..30d7ffb1e04c 100644 > --- a/drivers/tty/hvc/hvc_xen.c > +++ b/drivers/tty/hvc/hvc_xen.c > @@ -86,6 +86,11 @@ static int __write_console(struct xencons_info *xencons, > cons = intf->out_cons; > prod = intf->out_prod; > mb(); /* update queue values before going on */ > + > + if (WARN_ONCE((prod - cons) > sizeof(intf->out), > + "Illegal ring page indices")) > + return -EINVAL; How nice, you just rebooted on panic-on-warn systems :( > + > BUG_ON((prod - cons) > sizeof(intf->out)); Why keep this line? Please just fix this up properly, if userspace can trigger this, then both the WARN_ON() and BUG_ON() are not correct and need to be correctly handled. > > while ((sent < len) && ((prod - cons) < sizeof(intf->out))) > @@ -114,7 +119,10 @@ static int domU_write_console(uint32_t vtermno, const char *data, int len) > */ > while (len) { > int sent = __write_console(cons, data, len); > - > + > + if (sent < 0) > + return sent; > + > data += sent; > len -= sent; > > @@ -138,7 +146,10 @@ static int domU_read_console(uint32_t vtermno, char *buf, int len) > cons = intf->in_cons; > prod = intf->in_prod; > mb(); /* get pointers before reading ring */ > - BUG_ON((prod - cons) > sizeof(intf->in)); > + > + if (WARN_ONCE((prod - cons) > sizeof(intf->in), > + "Illegal ring page indices")) > + return -EINVAL; Same here, you still just paniced a machine :( thanks, greg k-h ``` ## Re: \[PATCH 8/8\] xen/hvc: replace BUG\_ON() with negative return value - From: Juergen Gross \ - Date: 2021-05-13 10:35:29 - Message: https://ratatoskr.run/linuxppc-dev/2021/05/1376565.md ```text On 13.05.21 12:25, Greg Kroah-Hartman wrote: > On Thu, May 13, 2021 at 12:03:02PM +0200, Juergen Gross wrote: >> Xen frontends shouldn't BUG() in case of illegal data received from >> their backends. So replace the BUG_ON()s when reading illegal data from >> the ring page with negative return values. >> >> Signed-off-by: Juergen Gross >> --- >> drivers/tty/hvc/hvc_xen.c | 15 +++++++++++++-- >> 1 file changed, 13 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/tty/hvc/hvc_xen.c b/drivers/tty/hvc/hvc_xen.c >> index 92c9a476defc..30d7ffb1e04c 100644 >> --- a/drivers/tty/hvc/hvc_xen.c >> +++ b/drivers/tty/hvc/hvc_xen.c >> @@ -86,6 +86,11 @@ static int __write_console(struct xencons_info *xencons, >> cons = intf->out_cons; >> prod = intf->out_prod; >> mb(); /* update queue values before going on */ >> + >> + if (WARN_ONCE((prod - cons) > sizeof(intf->out), >> + "Illegal ring page indices")) >> + return -EINVAL; > > How nice, you just rebooted on panic-on-warn systems :( > >> + >> BUG_ON((prod - cons) > sizeof(intf->out)); > > Why keep this line? Failed to delete it, sorry. > > Please just fix this up properly, if userspace can trigger this, then > both the WARN_ON() and BUG_ON() are not correct and need to be correctly > handled. It can be triggered by the console backend, but I agree a WARN isn't the way to go here. Juergen ``` ## Re: \[PATCH 0/8\] xen: harden frontends against malicious backends - From: Marek Marczykowski-Górecki \ - Date: 2021-05-21 10:43:15 - Message: https://ratatoskr.run/linuxppc-dev/2021/05/1387361.md ```text On Thu, May 13, 2021 at 12:02:54PM +0200, Juergen Gross wrote: > Xen backends of para-virtualized devices can live in dom0 kernel, dom0 > user land, or in a driver domain. This means that a backend might > reside in a less trusted environment than the Xen core components, so > a backend should not be able to do harm to a Xen guest (it can still > mess up I/O data, but it shouldn't be able to e.g. crash a guest by > other means or cause a privilege escalation in the guest). > > Unfortunately many frontends in the Linux kernel are fully trusting > their respective backends. This series is starting to fix the most > important frontends: console, disk and network. > > It was discussed to handle this as a security problem, but the topic > was discussed in public before, so it isn't a real secret. Is it based on patches we ship in Qubes[1] and also I've sent here some years ago[2]? I see a lot of similarities. If not, you may want to compare them. [1] https://github.com/QubesOS/qubes-linux-kernel/ [2] https://lists.xenproject.org/archives/html/xen-devel/2018-04/msg02336.html > Juergen Gross (8): > xen: sync include/xen/interface/io/ring.h with Xen's newest version > xen/blkfront: read response from backend only once > xen/blkfront: don't take local copy of a request from the ring page > xen/blkfront: don't trust the backend response data blindly > xen/netfront: read response from backend only once > xen/netfront: don't read data from request on the ring page > xen/netfront: don't trust the backend response data blindly > xen/hvc: replace BUG_ON() with negative return value > > drivers/block/xen-blkfront.c | 118 +++++++++----- > drivers/net/xen-netfront.c | 184 ++++++++++++++------- > drivers/tty/hvc/hvc_xen.c | 15 +- > include/xen/interface/io/ring.h | 278 ++++++++++++++++++-------------- > 4 files changed, 369 insertions(+), 226 deletions(-) > > -- > 2.26.2 > > -- Best Regards, Marek Marczykowski-Górecki Invisible Things Lab ```