veikk_set_veikk_screen_size: Deserialization reads uninitialized values

[cebtenzzre]

Issue Description

The following deserialization code (in veikk_set_veikk_screen_size) upsets clang-tidy.

veikk-linux-driver/veikk_modparms.c

Lines 51 to 57 in 7ccdfb1

	if((error = kstrtouint(val, 10, &ss)))
	return error;
	// check that entire number is zero, or that width, height > 0 (see desc)
	screen_size = *(struct veikk_screen_size *)&ss;
	if(ss && (screen_size.width<=0 || screen_size.height<=0))
	return -EINVAL;

This is what clang-tidy says about it:

veikk_modparms.c:56:57: warning: The left operand of '<=' is a garbage value [clang-analyzer-core.UndefinedBinaryOperatorResult]
    if(ss && (screen_size.width<=0 || screen_size.height<=0))
                                                        ^

height is a garbage value because ss (u32, 4 bytes) is reinterpreted as a struct veikk_screen_size (16 bytes). Only x is actually initialized by line 55.
height is a "garbage value" because of the way two 16-bit integers are unpacked from a 32-bit integer using type punning. This probably works, but there are ways to do this that aren't considered unspecified behavior - such as shifting and masking.

In revision 81c7207, veikk_set_veikk_screen_size used veikk_ss_to_rect, which didn't have this problem.

Versions

This is a regression introduced by commit 73be60c.

[jlam55555]

The casting done in 73be60c was done mostly out of convenience and a (naive?) thought that gcc's behavior should be consistent across most devices. However, I will revert it to the more type-safe casting once I get time to work on this again.

By the way, @cebtenzzre, did this cause any actual problems for you? Are you building your kernel with clang? Asking out of curiosity. If you have any advice about the safety of this cast, please let me know. AFAIK, struct veikk_screen_size should be 4 bytes by any reasonable compiler implementation -- any thoughts on why clang says 16 bytes?

[cebtenzzre]

Sorry, I was originally confused between the two definitions of veikk_screen_size: It refers to both a variable of type struct veikk_rect AND an unrelated type with some overlap in the names of its members (struct veikk_screen_size). This code isn't as bad as it looked to me at first, but I still think clang-tidy makes a valid point - this is unspecified behavior.

I'm more familiar with C++, where type punning like this is undefined behavior and variable names can't be the same as struct names.

And I actually haven't used this driver yet, I just threw a static analyzer at it to see if it was of sufficient quality to consider using.

[jlam55555]

"This code isn't as bad as it looked to me at first" -- haha, I guess that's a good thing to hear. I agree about fixing the UB though, and the poor choice of variable name. Will fix later.