Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Replace fake-flex array with flex-array member in drivers/hid/intel-ish-hid/ishtp-hid.h #333

Open
GustavoARSilva opened this issue Aug 1, 2023 · 2 comments
Open

Replace fake-flex array with flex-array member in drivers/hid/intel-ish-hid/ishtp-hid.h #333

GustavoARSilva opened this issue Aug 1, 2023 · 2 comments
Labels
[Idiom] fake flexible array [Refactor] 1-element array Conversion away from one-element array

Comments

@GustavoARSilva
Copy link
Collaborator 

diff -u -p a/drivers/hid/intel-ish-hid/ishtp-hid.h b/drivers/hid/intel-ish-hid/ishtp-hid.h
--- a/drivers/hid/intel-ish-hid/ishtp-hid.h
+++ b/drivers/hid/intel-ish-hid/ishtp-hid.h
@@ -58,9 +58,9 @@ struct report_list {
        uint8_t num_of_reports;
        uint8_t flags;
        struct {
               uint16_t        size_of_report;

                uint8_t report[1];
       } __packed reports[1];
 } __packed;
@kees
Copy link

kees commented Feb 6, 2024

A flexible array of flexible arrays. :(

@kees
Copy link

kees commented Aug 23, 2024

https://lore.kernel.org/lkml/AS8PR02MB723760CB93942370E92F00638BF72@AS8PR02MB7237.eurprd02.prod.outlook.com/

intel-lab-lkp pushed a commit to intel-lab-lkp/linux that referenced this issue Sep 23, 2024
@intel-lab-lkp
HID: ishtp-hid-client: replace fake-flex arrays with flex-array members
14ff1f6 
One-element arrays as fake flex arrays are deprecated[1] as the kernel
has switched to C99 flexible-array members instead. This case, however,
has more complexity because it is a flexible array of flexible arrays
and this patch needs to be ready to enable the new compiler flag
-Wflex-array-member-not-at-end (coming in GCC-14) globally.

So, define a new struct type for the single reports:

struct report {
	uint16_t size;
	struct hostif_msg_hdr msg;
} __packed;

but without the payload (flex array) in it. And add this payload to the
"hostif_msg" structure. This way, in the "report_list" structure we can
declare a flex array of single reports which now do not contain another
flex array.

struct report_list {
	[...]
        struct report reports[];
} __packed;

Therefore, the "struct hostif_msg" is now made up of a header and a
payload. And the "struct report" uses only the "hostif_msg" header.
The perfect solution would be for the "report" structure to use the
whole "hostif_msg" structure but this is not possible due to nested
flexible arrays. Anyway, the end result is equivalent since this patch
does attempt to change the behaviour of the code.

Now as well, we have more clarity after the cast from the raw bytes to
the new structures. Refactor the code accordingly to use the new
structures.

Also, use "container_of()" whenever we need to retrieve a pointer to
the flexible structure, through which we can access the flexible array
if needed.

Link: https://www.kernel.org/doc/html/next/process/deprecated.html#zero-length-and-one-element-arrays [1]
Closes: KSPP#333
Signed-off-by: Erick Archer <erick.archer@outlook.com>
Link: https://lore.kernel.org/r/AS8PR02MB723760CB93942370E92F00638BF72@AS8PR02MB7237.eurprd02.prod.outlook.com
[kees: tweaked commit log and dropped struct_size() uses]
Signed-off-by: Kees Cook <kees@kernel.org>
intel-lab-lkp pushed a commit to intel-lab-lkp/linux that referenced this issue Oct 8, 2024
HID: ishtp-hid-client: replace fake-flex arrays with flex-array members
63cafaf 
One-element arrays as fake flex arrays are deprecated[1] as the kernel
has switched to C99 flexible-array members instead. This case, however,
has more complexity because it is a flexible array of flexible arrays
and this patch needs to be ready to enable the new compiler flag
-Wflex-array-member-not-at-end (coming in GCC-14) globally.

So, define a new struct type for the single reports:

struct report {
	uint16_t size;
	struct hostif_msg_hdr msg;
} __packed;

but without the payload (flex array) in it. And add this payload to the
"hostif_msg" structure. This way, in the "report_list" structure we can
declare a flex array of single reports which now do not contain another
flex array.

struct report_list {
	[...]
        struct report reports[];
} __packed;

Therefore, the "struct hostif_msg" is now made up of a header and a
payload. And the "struct report" uses only the "hostif_msg" header.
The perfect solution would be for the "report" structure to use the
whole "hostif_msg" structure but this is not possible due to nested
flexible arrays. Anyway, the end result is equivalent since this patch
does attempt to change the behaviour of the code.

Now as well, we have more clarity after the cast from the raw bytes to
the new structures. Refactor the code accordingly to use the new
structures.

Also, use "container_of()" whenever we need to retrieve a pointer to
the flexible structure, through which we can access the flexible array
if needed.

Link: https://www.kernel.org/doc/html/next/process/deprecated.html#zero-length-and-one-element-arrays [1]
Closes: KSPP#333
Signed-off-by: Erick Archer <erick.archer@outlook.com>
Link: https://lore.kernel.org/r/AS8PR02MB723760CB93942370E92F00638BF72@AS8PR02MB7237.eurprd02.prod.outlook.com
[kees: tweaked commit log and dropped struct_size() uses]
Signed-off-by: Kees Cook <kees@kernel.org>
Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
Signed-off-by: Jiri Kosina <jkosina@suse.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
[Idiom] fake flexible array [Refactor] 1-element array Conversion away from one-element array
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@GustavoARSilva @kees