| Message ID | 20210513100302.22027-9-jgross@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen: harden frontends against malicious backends | expand |
| Related | show |
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 <jgross@suse.com> > --- > 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)]; >
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 <jgross@suse.com> >> --- >> 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
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 <jgross@suse.com> > --- > 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
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 <jgross@suse.com> >> --- >> 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
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)];
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 <jgross@suse.com> --- drivers/tty/hvc/hvc_xen.c | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-)