console: disallow opening for writing when no write method defined
Subsystems (4)
PATCH 1/4 console: disallow opening for writing when no write method defined
Some consoles, like the input console (usually /dev/cs0), don't feature a puts or putc callback. Trying to echo out of them would thus crash:
barebox@Embest MarS Board i.MX6Dual:/ echo -a /dev/cs0 prefetch abort pc : [<00000004>] lr : [<4fd05071>] WARNING: [<...>] (fops_write+0xd/0x10) WARNING: [<...>] (devfs_write+0x21/0x2a) WARNING: [<...>] (__write+0xcb/0xf0) WARNING: [<...>] (write+0x2d/0x68) WARNING: [<...>] (dputc+0x31/0x34) WARNING: [<...>] (do_echo+0xcb/0x144)
Fix this by only allowing open(.., O_WRONLY) or open(..., O_RDWR) when puts is defined. Consoles defining putc are covered by this as well as those have putc-calling __console_puts assigned as their puts when they are registered. Now echo -a /dev/cs0 would yield:
open: Operation not permitted
--- common/console.c | 3 +++ 1 file changed, 3 insertions(+)
common/console.c +3 -0
@@ -272,6 +272,9 @@ static int fops_open(struct cdev *cdev, unsigned long flags) {struct console_device *priv = cdev->priv; +if ((flags & (O_WRONLY |O_RDWR )) && !priv->puts ) +return -EPERM ; +return console_open (priv); }
Signature · 7 lines
2.20.1 _______________________________________________ barebox mailing list [email protected] http://lists.infradead.org/mailman/listinfo/barebox
PATCH 2/4 input: set console input name as input
The input console is usually called /dev/cs0 and devinfo doesn't yield any extra information what this device is about. Rename it for clarity.
--- drivers/input/input.c | 2 ++ 1 file changed, 2 insertions(+)
drivers/input/input.c +2 -0
@@ -195,6 +195,8 @@ static int input_init(void) ic->console.tstc = input_console_tstc; ic->console.getc = input_console_getc; ic->console.f_active =CONSOLE_STDIN ; + ic->console.devid =DEVICE_ID_DYNAMIC ; + ic->console.devname ="input" ; ic->fifo =kfifo_alloc (32 ); ic->notifier.notify = input_console_notify;
Signature · 7 lines
2.20.1 _______________________________________________ barebox mailing list [email protected] http://lists.infradead.org/mailman/listinfo/barebox
PATCH 3/4 common: ubsan: ignore shifting one into sign bit
In reply to Ahmad FatoumThe __ubsan_handle_shift_out_of_bounds handler would be called for code shifting a one into the sign bit like (1 << 31), which is all too common in barebox. It's technically UB, but it's so prevalent that it's highly unlikely to be treated by a compiler as anything else than the standard-compliant (1U << 31).
Check for this case here and ignore it selectively.
--- lib/ubsan.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)
lib/ubsan.c +20 -0
@@ -382,6 +382,26 @@ void __ubsan_handle_shift_out_of_bounds(struct shift_out_of_bounds_data *data,if (suppress_report (&data->location))return ; +/* This handler would be called for code shifting a one into the +* sign bit like (1 << 31), which is all too common in barebox. +* It's technically UB, but it's so prevalent that it's highly +* unlikely to be treated by a compiler as anything else than the +* standard-compliant (1U << 31). Thus check for this case here +* and ignore it selectively +*/ +if (type_is_signed (lhs_type)) { +s_max lhs_int, rhs_int; + + lhs_int =get_signed_val (lhs_type, lhs); + rhs_int =get_signed_val (rhs_type, rhs); + +if (fls (lhs_int) + rhs_int ==type_bit_width (lhs_type)) { +pr_debug ("signed left shift of %lld by %lld ignored.\n" , + (s64 )lhs_int, (s64 )rhs_int); +return ; + } + } +ubsan_prologue (&data->location, &flags);val_to_string (rhs_str,sizeof (rhs_str), rhs_type, rhs);
Signature · 7 lines
2.20.1 _______________________________________________ barebox mailing list [email protected] http://lists.infradead.org/mailman/listinfo/barebox
Re: [PATCH v1 3/4] common: ubsan: ignore shifting one into sign bit
Hi Ahmad,
Quoted 10 lines · Ahmad Fatoum
The
__ubsan_handle_shift_out_of_boundshandler would be called for code
shifting a one into the sign bit like (1 << 31), which is all too common
in barebox. It's technically UB, but it's so prevalent that it's highly
unlikely to be treated by a compiler as anything else than the standard-compliant (1U << 31).Check for this case here and ignore it selectively.
Shouldn't we rather fix the the (1 << 31) to be (1U <<31)?
Quoted 33 lines
Signed-off-by Ahmad Fatoum <[email protected]>--- lib/ubsan.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)lib/ubsan.c +1
-0@@ -382,6 +382,26 @@ void __ubsan_handle_shift_out_of_bounds(struct shift_out_of_bounds_data *data, if (suppress_report(&data->location)) return; +/* This handler would be called for code shifting a one into the
lib/ubsan.c +3
-0+* sign bit like (1 << 31), which is all too common in barebox. +* It's technically UB, but it's so prevalent that it's highly +* unlikely to be treated by a compiler as anything else than the
lib/ubsan.c +10
-0+* standard-compliant (1U << 31). Thus check for this case here +* and ignore it selectively +*/ +if (type_is_signed (lhs_type)) { +s_max lhs_int, rhs_int; + + lhs_int =get_signed_val (lhs_type, lhs); + rhs_int =get_signed_val (rhs_type, rhs); + +if (fls (lhs_int) + rhs_int ==type_bit_width (lhs_type)){
lib/ubsan.c +1
-0+ pr_debug("signed left shift of %lld by %lldignored.\n",
lib/ubsan.c void __ubsan_handle_shift_out_of_bounds(struct shift_out_of_bounds_data *data, · line 407
Quoted 8 lines
lib/ubsan.c +5
-0+ (s64)lhs_int, (s64)rhs_int); +return ; + } + } +ubsan_prologue (&data->location, &flags);val_to_string (rhs_str,sizeof (rhs_str), rhs_type, rhs);
- rcz
_______________________________________________
barebox mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/barebox
Re: [PATCH v1 3/4] common: ubsan: ignore shifting one into sign bit
Hello Rouven,
Quoted 12 lines · Rouven Czerwinski
Hi Ahmad,
Quoted 9 lines · Ahmad Fatoum
The
__ubsan_handle_shift_out_of_boundshandler would be called for code
shifting a one into the sign bit like (1 << 31), which is all too common
in barebox. It's technically UB, but it's so prevalent that it's highly
unlikely to be treated by a compiler as anything else than the standard-compliant (1U << 31).
Quoted 3 lines
Quoted 1 line
Check for this case here and ignore it selectively.
Shouldn't we rather fix the the (1 << 31) to be (1U <<31)?
Well, we could of course convert all bitwise operations to act on unsigned integers or to use the BIT() macro, but that's possibly thousands of instances. A very tedious work I'd rather not be doing... :D
Quoted 51 lines
Quoted 34 lines
Signed-off-by Ahmad Fatoum <[email protected]>--- lib/ubsan.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)lib/ubsan.c +1
-0@@ -382,6 +382,26 @@ void __ubsan_handle_shift_out_of_bounds(struct shift_out_of_bounds_data *data, if (suppress_report(&data->location)) return; +/* This handler would be called for code shifting a one into the
lib/ubsan.c +3
-0+* sign bit like (1 << 31), which is all too common in barebox. +* It's technically UB, but it's so prevalent that it's highly +* unlikely to be treated by a compiler as anything else than the
lib/ubsan.c +10
-0+* standard-compliant (1U << 31). Thus check for this case here +* and ignore it selectively +*/ +if (type_is_signed (lhs_type)) { +s_max lhs_int, rhs_int; + + lhs_int =get_signed_val (lhs_type, lhs); + rhs_int =get_signed_val (rhs_type, rhs); + +if (fls (lhs_int) + rhs_int ==type_bit_width (lhs_type)){
lib/ubsan.c +1
-0+ pr_debug("signed left shift of %lld by %lldignored.\n",
lib/ubsan.c void __ubsan_handle_shift_out_of_bounds(struct shift_out_of_bounds_data *data, · line 407
Quoted 8 lines
lib/ubsan.c +5
-0+ (s64)lhs_int, (s64)rhs_int); +return ; + } + } +ubsan_prologue (&data->location, &flags);val_to_string (rhs_str,sizeof (rhs_str), rhs_type, rhs);- rcz
_______________________________________________
barebox mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/barebox
Signature · 9 lines
Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | _______________________________________________ barebox mailing list [email protected] http://lists.infradead.org/mailman/listinfo/barebox
PATCH 4/4 USB: gadget: ACM: don't announce V.25ter support
In reply to Ahmad Fatoumbarebox currently announces support for ITU V.25ter AT commands, but doesn't handle them specially when they arrive. Instead they are passed as is to the sole barebox input console, where it may interfere with valid user input.
This is especially annoying as ModemManager probes ttyACM devices that announce their AT command support. So even when not using the ttyACM device at all, the other UART ports are affected.
Fix this by ceasing to announce USB_CDC_ACM_PROTO_AT_V25TER as function protocol. After applying this patch, I can't see any spurious AT or ~x~ symbols on the console anymore.
--- drivers/usb/gadget/f_acm.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/usb/gadget/f_acm.c +2 -2
@@ -104,7 +104,7 @@ acm_iad_descriptor = { .bInterfaceCount = 2, // control + data .bFunctionClass = USB_CLASS_COMM, .bFunctionSubClass = USB_CDC_SUBCLASS_ACM, - .bFunctionProtocol = USB_CDC_ACM_PROTO_AT_V25TER, + .bFunctionProtocol = USB_CDC_PROTO_NONE,/* .iFunction = DYNAMIC */ };
@@ -116,7 +116,7 @@ static struct usb_interface_descriptor acm_control_interface_desc = { .bNumEndpoints = 1, .bInterfaceClass = USB_CLASS_COMM, .bInterfaceSubClass = USB_CDC_SUBCLASS_ACM, - .bInterfaceProtocol = USB_CDC_ACM_PROTO_AT_V25TER, + .bInterfaceProtocol = USB_CDC_PROTO_NONE,/* .iInterface = DYNAMIC */ };
Signature · 7 lines
2.20.1 _______________________________________________ barebox mailing list [email protected] http://lists.infradead.org/mailman/listinfo/barebox
Re: [PATCH v1 4/4] USB: gadget: ACM: don't announce V.25ter support
Quoted 11 lines · Ahmad Fatoum
barebox currently announces support for ITU V.25ter AT commands, but doesn't handle them specially when they arrive. Instead they are passed as is to the sole barebox input console, where it may interfere with valid user input.
This is especially annoying as ModemManager probes ttyACM devices that announce their AT command support. So even when not using the ttyACM device at all, the other UART ports are affected.Fix this by ceasing to announce
USB_CDC_ACM_PROTO_AT_V25TERas function protocol. After applying this patch, I can't see any spurious AT or ~x~ symbols on the console anymore.
I've also looked into the possibility of barebox emulating a USB-Serial port and showing up as a ttyUSB device under Linux.
Apparently the two most popular chips are FTDI's and Prolific's, but both have taken steps in the past to make their Windows drivers not work with cloned chips[1][2]. I figured the safest to emulate would be the chip used safely in these Chinese Arduino Knock-offs because they're so prevalent and stumbled upon the CH340. Its drivers are mainline for Linux, but for other platforms, they need to be downloaded manually.
Seeing that the change to 'fix' barebox ttyACM is just two lines, I settled for that instead. It might be possible to get rid of the control interface descriptor altogether, but I don't know whether it would impact the portability across operating systems, so I've opted for this less invasive patch. Only tested on Linux v5.2 so far.
Cheers
Ahmad
[1]: https://hackaday.com/2016/02/01/ftdi-drivers-break-fake-chips-again/
[2]: https://www.eevblog.com/forum/reviews/ftdi-driver-kills-fake-ftdi-ft232/msg534439/#msg534439
Quoted 31 lines
--- drivers/usb/gadget/f_acm.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)drivers/usb/gadget/f_acm.c +2
-2@@ -104,7 +104,7 @@ acm_iad_descriptor = { .bInterfaceCount = 2, // control + data .bFunctionClass = USB_CLASS_COMM, .bFunctionSubClass = USB_CDC_SUBCLASS_ACM, - .bFunctionProtocol = USB_CDC_ACM_PROTO_AT_V25TER, + .bFunctionProtocol = USB_CDC_PROTO_NONE,/* .iFunction = DYNAMIC */ };@@ -116,7 +116,7 @@ static struct usb_interface_descriptor acm_control_interface_desc = { .bNumEndpoints = 1, .bInterfaceClass = USB_CLASS_COMM, .bInterfaceSubClass = USB_CDC_SUBCLASS_ACM, - .bInterfaceProtocol = USB_CDC_ACM_PROTO_AT_V25TER, + .bInterfaceProtocol = USB_CDC_PROTO_NONE,/* .iInterface = DYNAMIC */ };
Signature · 9 lines
Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | _______________________________________________ barebox mailing list [email protected] http://lists.infradead.org/mailman/listinfo/barebox
Re: [PATCH v1 4/4] USB: gadget: ACM: don't announce V.25ter support
Quoted 22 lines · Ahmad Fatoum
Quoted 11 lines · Ahmad Fatoum
barebox currently announces support for ITU V.25ter AT commands, but doesn't handle them specially when they arrive. Instead they are passed as is to the sole barebox input console, where it may interfere with valid user input.
This is especially annoying as ModemManager probes ttyACM devices that announce their AT command support. So even when not using the ttyACM device at all, the other UART ports are affected.Fix this by ceasing to announce
USB_CDC_ACM_PROTO_AT_V25TERas function protocol. After applying this patch, I can't see any spurious AT or ~x~ symbols on the console anymore.I've also looked into the possibility of barebox emulating a USB-Serial port and showing up as a ttyUSB device under Linux.
Apparently the two most popular chips are FTDI's and Prolific's, but both have taken steps in the past to make their Windows drivers not work with cloned chips[1][2]. I figured the safest to emulate would be the chip used safely in these Chinese Arduino Knock-offs because they're so prevalent and stumbled upon the CH340. Its drivers are mainline for Linux, but for other platforms, they need to be downloaded manually.
Quoted 4 lines
Seeing that the change to 'fix' barebox ttyACM is just two lines, I settled for that instead. It might be possible to get rid of the control interface descriptor altogether, but I don't know whether it would impact the portability across operating systems, so I've opted for this less invasive patch.
Getting rid of the control descriptor makes Linux ttyACM disappear, so I would stick to this patch.
Quoted 1 line
Only tested on Linux v5.2 so far.
Tested working as well on Windows 10 Home and macOS Mojave.
Quoted 39 lines
Cheers
Ahmad[1]: https://hackaday.com/2016/02/01/ftdi-drivers-break-fake-chips-again/
[2]: https://www.eevblog.com/forum/reviews/ftdi-driver-kills-fake-ftdi-ft232/msg534439/#msg534439Quoted 31 lines
--- drivers/usb/gadget/f_acm.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)drivers/usb/gadget/f_acm.c +2
-2@@ -104,7 +104,7 @@ acm_iad_descriptor = { .bInterfaceCount = 2, // control + data .bFunctionClass = USB_CLASS_COMM, .bFunctionSubClass = USB_CDC_SUBCLASS_ACM, - .bFunctionProtocol = USB_CDC_ACM_PROTO_AT_V25TER, + .bFunctionProtocol = USB_CDC_PROTO_NONE,/* .iFunction = DYNAMIC */ };@@ -116,7 +116,7 @@ static struct usb_interface_descriptor acm_control_interface_desc = { .bNumEndpoints = 1, .bInterfaceClass = USB_CLASS_COMM, .bInterfaceSubClass = USB_CDC_SUBCLASS_ACM, - .bInterfaceProtocol = USB_CDC_ACM_PROTO_AT_V25TER, + .bInterfaceProtocol = USB_CDC_PROTO_NONE,/* .iInterface = DYNAMIC */ };
Signature · 9 lines
Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | _______________________________________________ barebox mailing list [email protected] http://lists.infradead.org/mailman/listinfo/barebox
Sascha Hauer
AppliedQuoted 18 lines · Ahmad Fatoum
Some consoles, like the input console (usually
/dev/cs0), don't feature a puts or putc callback. Trying to echo out of them would thus crash:barebox@Embest MarS Board i.MX6Dual:/ echo -a /dev/cs0 prefetch abort pc : [<00000004>] lr : [<4fd05071>] WARNING: [<...>] (fops_write+0xd/0x10) WARNING: [<...>] (devfs_write+0x21/0x2a) WARNING: [<...>] (__write+0xcb/0xf0) WARNING: [<...>] (write+0x2d/0x68) WARNING: [<...>] (dputc+0x31/0x34) WARNING: [<...>] (do_echo+0xcb/0x144)Fix this by only allowing
open(.., O_WRONLY)oropen(..., O_RDWR)when puts is defined. Consoles defining putc are covered by this as well as those have putc-calling__console_putsassigned as their puts when they are registered. Now echo -a/dev/cs0would yield:
Quoted 4 lines
open: Operation not permitted
Signed-off-by Ahmad Fatoum <[email protected]>---
Applied, thanks
Sascha
Signature · 9 lines
Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | _______________________________________________ barebox mailing list [email protected] http://lists.infradead.org/mailman/listinfo/barebox