[PATCH v1 1/4] console: disallow opening for writing when no write method defined

[PATCH v1 1/4] console: disallow opening for writing when no write method defined

[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) 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

Signed-off-by: Ahmad Fatoum <ahmad@a3f.at>
---
 common/console.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/common/console.c b/common/console.c
index d04aae58f96e..e6e029848d41 100644
--- a/common/console.c
+++ b/common/console.c
@@ -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);
 }
 
-- 
2.20.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v1 2/4] input: set console input name as input

[Ahmad Fatoum]

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.

Signed-off-by: Ahmad Fatoum <ahmad@a3f.at>
---
 drivers/input/input.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/input/input.c b/drivers/input/input.c
index 14e44d137826..1e8f6e178e0d 100644
--- a/drivers/input/input.c
+++ b/drivers/input/input.c
@@ -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;
-- 
2.20.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v1 3/4] common: ubsan: ignore shifting one into sign bit

[Ahmad Fatoum]

The __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.

Signed-off-by: Ahmad Fatoum <ahmad@a3f.at>
---
 lib/ubsan.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/lib/ubsan.c b/lib/ubsan.c
index 89ca6e580bce..41a5731dda66 100644
--- a/lib/ubsan.c
+++ b/lib/ubsan.c
@@ -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);
-- 
2.20.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v1 3/4] common: ubsan: ignore shifting one into sign bit

[Rouven Czerwinski]

Hi Ahmad,

On Fri, 2019-09-20 at 09:58 +0200, Ahmad Fatoum wrote:
> The __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.

Shouldn't we rather fix the the (1 << 31) to be (1U <<31)?

> Signed-off-by: Ahmad Fatoum <ahmad@a3f.at>
> ---
>  lib/ubsan.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/lib/ubsan.c b/lib/ubsan.c
> index 89ca6e580bce..41a5731dda66 100644
> --- a/lib/ubsan.c
> +++ b/lib/ubsan.c
> @@ -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);

- rcz


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v1 3/4] common: ubsan: ignore shifting one into sign bit

[Ahmad Fatoum]

Hello Rouven,

On 9/21/19 9:47 AM, Rouven Czerwinski wrote:
> Hi Ahmad,
> 
> On Fri, 2019-09-20 at 09:58 +0200, Ahmad Fatoum wrote:
>> The __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.
> 
> 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

> 
>> Signed-off-by: Ahmad Fatoum <ahmad@a3f.at>
>> ---
>>  lib/ubsan.c | 20 ++++++++++++++++++++
>>  1 file changed, 20 insertions(+)
>>
>> diff --git a/lib/ubsan.c b/lib/ubsan.c
>> index 89ca6e580bce..41a5731dda66 100644
>> --- a/lib/ubsan.c
>> +++ b/lib/ubsan.c
>> @@ -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);
> 
> - rcz
> 
> 
> _______________________________________________
> barebox mailing list
> barebox@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/barebox
> 

-- 
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
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v1 4/4] USB: gadget: ACM: don't announce V.25ter support

[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_V25TER as function
protocol. After applying this patch, I can't see any spurious AT or ~x~
symbols on the console anymore.

Cc: <bst@pengutronix.de>
Cc: <jlu@pengutronix.de>
Signed-off-by: Ahmad Fatoum <ahmad@a3f.at>
---
 drivers/usb/gadget/f_acm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/f_acm.c b/drivers/usb/gadget/f_acm.c
index cba59b15859b..42a2b03ad291 100644
--- a/drivers/usb/gadget/f_acm.c
+++ b/drivers/usb/gadget/f_acm.c
@@ -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 */
 };
 
-- 
2.20.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v1 4/4] USB: gadget: ACM: don't announce V.25ter support

[Ahmad Fatoum]

On 9/20/19 9:58 AM, Ahmad Fatoum wrote:
> 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_V25TER as 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

> 
> Cc: <bst@pengutronix.de>
> Cc: <jlu@pengutronix.de>
> Signed-off-by: Ahmad Fatoum <ahmad@a3f.at>
> ---
>  drivers/usb/gadget/f_acm.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/gadget/f_acm.c b/drivers/usb/gadget/f_acm.c
> index cba59b15859b..42a2b03ad291 100644
> --- a/drivers/usb/gadget/f_acm.c
> +++ b/drivers/usb/gadget/f_acm.c
> @@ -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 */
>  };
>  
> 

-- 
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
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v1 4/4] USB: gadget: ACM: don't announce V.25ter support

[Ahmad Fatoum]

On 9/20/19 10:51 AM, Ahmad Fatoum wrote:
> On 9/20/19 9:58 AM, Ahmad Fatoum wrote:
>> 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_V25TER as 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.

Getting rid of the control descriptor makes Linux ttyACM disappear, so I would
stick to this patch.

> Only tested on Linux v5.2 so far.

Tested working as well on Windows 10 Home and macOS Mojave.

> 
> 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
> 
>>
>> Cc: <bst@pengutronix.de>
>> Cc: <jlu@pengutronix.de>
>> Signed-off-by: Ahmad Fatoum <ahmad@a3f.at>
>> ---
>>  drivers/usb/gadget/f_acm.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/f_acm.c b/drivers/usb/gadget/f_acm.c
>> index cba59b15859b..42a2b03ad291 100644
>> --- a/drivers/usb/gadget/f_acm.c
>> +++ b/drivers/usb/gadget/f_acm.c
>> @@ -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 */
>>  };
>>  
>>
> 

-- 
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
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v1 1/4] console: disallow opening for writing when no write method defined

[Sascha Hauer]

On Fri, Sep 20, 2019 at 09:58:10AM +0200, Ahmad Fatoum wrote:
> 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
> 
> Signed-off-by: Ahmad Fatoum <ahmad@a3f.at>
> ---

Applied, thanks

Sascha

-- 
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
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox