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

[Ahmad Fatoum <a.fatoum@pengutronix.de>]

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