Skip to contentSkip to conversation

xen/hvc: replace BUG_ON() with negative return value

PATCH In review Juergen Gross 3 participants 4 messages Jul 7, 2021

Subsystems (2)
TTY layer and serial drivers Hypervisor virtual console driver
lore
Share & tools
lore

Juergen Gross

author
#0 lore raw

PATCH v2 1/1 xen/hvc: replace BUG_ON() with negative return value

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 <[email protected]>

---
V2:
- drop BUG_ON() (Christophe Leroy, Greg Kroah-Hartmann)
- replace WARN_ONCE() by pr_err_once() (Greg Kroah-Hartmann)
- break out from original series

---
 drivers/tty/hvc/hvc_xen.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)
drivers/tty/hvc/hvc_xen.c +14 -3
index 92c9a476defc..8f143c09a169 100644
@@ -86,7 +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 */
-	BUG_ON((prod - cons) > sizeof(intf->out));
+
+	if ((prod - cons) > sizeof(intf->out)) {
+		pr_err_once("xencons: Illegal ring page indices");
+		return -EINVAL;
+	}
 
 	while ((sent < len) && ((prod - cons) < sizeof(intf->out)))
 		intf->out[MASK_XENCONS_IDX(prod++, intf->out)] = data[sent++];

@@ -114,7 +118,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 +145,11 @@ 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 ((prod - cons) > sizeof(intf->in)) {
+		pr_err_once("xencons: Illegal ring page indices");
+		return -EINVAL;
+	}
 
 	while (cons != prod && recv < len)
 		buf[recv++] = intf->in[MASK_XENCONS_IDX(cons++, intf->in)];
Signature · 1 line
2.26.2
Reviewed-by on v2 1/1
#1 lore raw
Quoted 5 lines · Juergen Gross

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 <[email protected]>
Reviewed-by Jan Beulich <[email protected]>

drivers/tty/hvc/hvc_xen.c static int __write_console(struct xencons_info *xencons, · line 88

Quoted 6 lines
drivers/tty/hvc/hvc_xen.c +0 -0
@@ -86,7 +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 */

Largely unrelated note: While in general the barriers here may want switching to virt_*mb(), this particular one looks to be too heavy
anyway: a read barrier is all that's needed here afaict, just like there's only a write barrier between ring contents and producer writing in __write_console().

And btw, since I've got puzzled by the linuxppc-dev@ in the recipients list, I did look up relevant entries in ./MAINTAINERS. Shouldn't the file be part of "XEN HYPERVISOR INTERFACE"?

Jan

author
#2 lore raw
Quoted 9 lines · Jan Beulich
Quoted 5 lines · Juergen Gross

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 <[email protected]>
Reviewed-by Jan Beulich <[email protected]>
Quoted 12 lines

drivers/tty/hvc/hvc_xen.c static int __write_console(struct xencons_info *xencons, · line 88

Quoted 6 lines
drivers/tty/hvc/hvc_xen.c +0 -0
@@ -86,7 +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 */

Largely unrelated note: While in general the barriers here may want switching to virt_*mb(), this particular one looks to be too heavy
anyway: a read barrier is all that's needed here afaict, just like there's only a write barrier between ring contents and producer writing in __write_console().

I agree.

Quoted 3 lines

And btw, since I've got puzzled by the linuxppc-dev@ in the recipients list, I did look up relevant entries in ./MAINTAINERS. Shouldn't the file be part of "XEN HYPERVISOR INTERFACE"?

I wouldn't mind. Greg, Jiri, what do you think?

Juergen

maintainer
#3 lore raw
Quoted 5 lines · Juergen Gross
Quoted 3 lines

And btw, since I've got puzzled by the linuxppc-dev@ in the recipients list, I did look up relevant entries in ./MAINTAINERS. Shouldn't the file be part of "XEN HYPERVISOR INTERFACE"?

I wouldn't mind. Greg, Jiri, what do you think?

/me concurs.

thanks,

Signature · 2 lines
js
suse labs