hostap: Add __counted_by for struct prism2_download_data and use struct_size()

PATCH In review Gustavo A. R. Silva 4 participants 9 messages Oct 9, 2023 – Oct 11, 2023 P54 wireless driver

Share & tools

Gustavo A. R. Silva

author

2023-10-09 19:41 UTC #0 lore raw

PATCH 1/1 hostap: Add __counted_by for struct prism2_download_data and use struct_size()

Prepare for the coming implementation by GCC and Clang of the __counted_by attribute. Flexible array members annotated with __counted_by can have their accesses bounds-checked at run-time via CONFIG_UBSAN_BOUNDS (for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family functions).

While there, use struct_size() helper, instead of the open-coded version, to calculate the size for the allocation of the whole flexible structure, including of course, the flexible-array member.

This code was found with the help of Coccinelle, and audited and fixed manually.

Signed-off-by Gustavo A. R. Silva <[email protected]>

---
 drivers/net/wireless/intersil/hostap/hostap_download.c | 3 +--
 drivers/net/wireless/intersil/hostap/hostap_wlan.h     | 2 +-
 2 files changed, 2 insertions(+), 3 deletions(-)

drivers/net/wireless/intersil/hostap/hostap_download.c +1 -2

index 3672291ced5c..5e5bada28b5b 100644
@@ -732,8 +732,7 @@ static int prism2_download(local_info_t *local,
 		goto out;
 	}
 
-	dl = kzalloc(sizeof(*dl) + param->num_areas *
-		     sizeof(struct prism2_download_data_area), GFP_KERNEL);
+	dl = kzalloc(struct_size(dl, data, param->num_areas), GFP_KERNEL);
 	if (dl == NULL) {
 		ret = -ENOMEM;
 		goto out;

drivers/net/wireless/intersil/hostap/hostap_wlan.h +1 -1

index c25cd21d18bd..f71c0545c0be 100644
@@ -617,7 +617,7 @@ struct prism2_download_data {
 		u32 addr; /* wlan card address */
 		u32 len;
 		u8 *data; /* allocated data */
-	} data[];
+	} data[] __counted_by(num_areas);
 };

Signature · 1 line

2.34.1

Kees Cook

2023-10-09 19:54 UTC #1 lore raw

Quoted 38 lines · Gustavo A. R. Silva

Prepare for the coming implementation by GCC and Clang of the __counted_by attribute. Flexible array members annotated with __counted_by can have their accesses bounds-checked at run-time via CONFIG_UBSAN_BOUNDS (for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family functions).
While there, use struct_size() helper, instead of the open-coded version, to calculate the size for the allocation of the whole flexible structure, including of course, the flexible-array member.
This code was found with the help of Coccinelle, and audited and fixed manually.
Signed-off-by Gustavo A. R. Silva <[email protected]>
---
 drivers/net/wireless/intersil/hostap/hostap_download.c | 3 +--
 drivers/net/wireless/intersil/hostap/hostap_wlan.h     | 2 +-
 2 files changed, 2 insertions(+), 3 deletions(-)
drivers/net/wireless/intersil/hostap/hostap_download.c +1 -2
index 3672291ced5c..5e5bada28b5b 100644
@@ -732,8 +732,7 @@ static int prism2_download(local_info_t *local,
 		goto out;
 	}
 
-	dl = kzalloc(sizeof(*dl) + param->num_areas *
-		     sizeof(struct prism2_download_data_area), GFP_KERNEL);
+	dl = kzalloc(struct_size(dl, data, param->num_areas), GFP_KERNEL);
 	if (dl == NULL) {
 		ret = -ENOMEM;
 		goto out;
drivers/net/wireless/intersil/hostap/hostap_wlan.h +0 -0
index c25cd21d18bd..f71c0545c0be 100644

drivers/net/wireless/intersil/hostap/hostap_wlan.h struct prism2_download_data { · line 621

Quoted 6 lines

drivers/net/wireless/intersil/hostap/hostap_wlan.h +1 -1

@@ -617,7 +617,7 @@ struct prism2_download_data {
 		u32 addr; /* wlan card address */
 		u32 len;
 		u8 *data; /* allocated data */
-	} data[];
+	} data[] __counted_by(num_areas);
 };

Should this be considered a v2 of:
https://lore.kernel.org/linux-hardening/[email protected]/

Yours is more complete since it includes the struct_size() change...

Signature · 1 line

Kees Cook

Gustavo A. R. Silva

2023-10-09 20:33 UTC #2 lore raw

Quoted 49 lines · Kees Cook

Quoted 38 lines · Gustavo A. R. Silva
Prepare for the coming implementation by GCC and Clang of the __counted_by attribute. Flexible array members annotated with __counted_by can have their accesses bounds-checked at run-time via CONFIG_UBSAN_BOUNDS (for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family functions).
While there, use struct_size() helper, instead of the open-coded version, to calculate the size for the allocation of the whole flexible structure, including of course, the flexible-array member.
This code was found with the help of Coccinelle, and audited and fixed manually.
Signed-off-by Gustavo A. R. Silva <[email protected]>
---
  drivers/net/wireless/intersil/hostap/hostap_download.c | 3 +--
  drivers/net/wireless/intersil/hostap/hostap_wlan.h     | 2 +-
  2 files changed, 2 insertions(+), 3 deletions(-)
drivers/net/wireless/intersil/hostap/hostap_download.c +1 -2
index 3672291ced5c..5e5bada28b5b 100644
@@ -732,8 +732,7 @@ static int prism2_download(local_info_t *local,
  		goto out;
  	}
 
-	dl = kzalloc(sizeof(*dl) + param->num_areas *
-		     sizeof(struct prism2_download_data_area), GFP_KERNEL);
+	dl = kzalloc(struct_size(dl, data, param->num_areas), GFP_KERNEL);
  	if (dl == NULL) {
  		ret = -ENOMEM;
  		goto out;
drivers/net/wireless/intersil/hostap/hostap_wlan.h +0 -0
index c25cd21d18bd..f71c0545c0be 100644
drivers/net/wireless/intersil/hostap/hostap_wlan.h struct prism2_download_data { · line 621
Quoted 6 lines
drivers/net/wireless/intersil/hostap/hostap_wlan.h +1 -1
@@ -617,7 +617,7 @@ struct prism2_download_data {
  		u32 addr; /* wlan card address */
  		u32 len;
  		u8 *data; /* allocated data */
-	} data[];
+	} data[] __counted_by(num_areas);
  };
Should this be considered a v2 of:
https://lore.kernel.org/linux-hardening/[email protected]/

Quoted 1 line

?

Oops... sorry, I forgot I had reviewed this patch of yours.

Quoted 3 lines

Yours is more complete since it includes the struct_size() change...

Should I resend this explicitly marking it as a v2?

--
Gustavo

Kees Cook

2023-10-09 20:42 UTC #3 lore raw

Quoted 53 lines · Gustavo A. R. Silva

Quoted 48 lines · Kees Cook
Quoted 37 lines · Gustavo A. R. Silva
Prepare for the coming implementation by GCC and Clang of the __counted_by attribute. Flexible array members annotated with __counted_by can have their accesses bounds-checked at run-time via CONFIG_UBSAN_BOUNDS (for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family functions).
While there, use struct_size() helper, instead of the open-coded version, to calculate the size for the allocation of the whole flexible structure, including of course, the flexible-array member.
This code was found with the help of Coccinelle, and audited and fixed manually.
Signed-off-by Gustavo A. R. Silva <[email protected]>
---
  drivers/net/wireless/intersil/hostap/hostap_download.c | 3 +--
  drivers/net/wireless/intersil/hostap/hostap_wlan.h     | 2 +-
  2 files changed, 2 insertions(+), 3 deletions(-)
drivers/net/wireless/intersil/hostap/hostap_download.c +1 -2
index 3672291ced5c..5e5bada28b5b 100644
@@ -732,8 +732,7 @@ static int prism2_download(local_info_t *local,
  		goto out;
  	}
-	dl = kzalloc(sizeof(*dl) + param->num_areas *
-		     sizeof(struct prism2_download_data_area), GFP_KERNEL);
+	dl = kzalloc(struct_size(dl, data, param->num_areas), GFP_KERNEL);
  	if (dl == NULL) {
  		ret = -ENOMEM;
  		goto out;
drivers/net/wireless/intersil/hostap/hostap_wlan.h +0 -0
index c25cd21d18bd..f71c0545c0be 100644
drivers/net/wireless/intersil/hostap/hostap_wlan.h struct prism2_download_data { · line 621
Quoted 6 lines
drivers/net/wireless/intersil/hostap/hostap_wlan.h +1 -1
@@ -617,7 +617,7 @@ struct prism2_download_data {
  		u32 addr; /* wlan card address */
  		u32 len;
  		u8 *data; /* allocated data */
-	} data[];
+	} data[] __counted_by(num_areas);
  };
Should this be considered a v2 of:
https://lore.kernel.org/linux-hardening/[email protected]/
Quoted 1 line
?

Quoted 7 lines

Oops... sorry, I forgot I had reviewed this patch of yours.
Quoted 3 lines
Yours is more complete since it includes the struct_size() change...
Should I resend this explicitly marking it as a v2?

I defer to Kalle -- your Subject is more accurate, so perhaps let's just let this stand?

Signature · 1 line

Kees Cook

Kalle Valo

2023-10-10 06:17 UTC #4 lore raw

Quoted 8 lines · Kees Cook

Quoted 8 lines
Quoted 4 lines
Should this be considered a v2 of:
https://lore.kernel.org/linux-hardening/[email protected]/
?
Oops... sorry, I forgot I had reviewed this patch of yours.
Quoted 1 line

Quoted 7 lines

Quoted 4 lines
Quoted 2 lines
Yours is more complete since it includes the struct_size() change...
Should I resend this explicitly marking it as a v2?
I defer to Kalle -- your Subject is more accurate, so perhaps let's just let this stand?

No need to resend. I marked Kees' version as Superseded so that I don't accidentally take the wrong version.

In the title "wifi:" is missing but I can add that during commit.

Signature · 3 lines

https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Gustavo A. R. Silva

2023-10-10 12:44 UTC #5 lore raw

Quoted 6 lines · Kalle Valo

No need to resend. I marked Kees' version as Superseded so that I don't accidentally take the wrong version.
In the title "wifi:" is missing but I can add that during commit.

Awesome. :)

Thanks, Kalle!
--
Gustavo

Kees Cook

Reviewed-by on v1 1/1

2023-10-10 22:50 UTC #6 lore raw

In reply to Gustavo A. R. Silva

Quoted 6 lines · Gustavo A. R. Silva

Prepare for the coming implementation by GCC and Clang of the __counted_by attribute. Flexible array members annotated with __counted_by can have their accesses bounds-checked at run-time via CONFIG_UBSAN_BOUNDS (for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family functions).

Quoted 8 lines

While there, use struct_size() helper, instead of the open-coded version, to calculate the size for the allocation of the whole flexible structure, including of course, the flexible-array member.
This code was found with the help of Coccinelle, and audited and fixed manually.
Signed-off-by Gustavo A. R. Silva <[email protected]>

I realize I didn't actually send my R-b for this thread, so here it is:

Reviewed-by Kees Cook <[email protected]>

Signature · 1 line

Kees Cook

Gustavo A. R. Silva

2023-10-10 23:24 UTC #7 lore raw

Quoted 7 lines

I realize I didn't actually send my R-b for this thread, so here it is:
Reviewed-by Kees Cook <[email protected]>
:)

Thanks! :)

--
Gustavo

Kalle Valo

2023-10-11 09:20 UTC #8 lore raw

Re: [next] wifi: hostap: Add __counted_by for struct prism2_download_data and use struct_size()

In reply to Gustavo A. R. Silva

Quoted 10 lines · Gustavo A. R. Silva

Prepare for the coming implementation by GCC and Clang of the __counted_by attribute. Flexible array members annotated with __counted_by can have their accesses bounds-checked at run-time via CONFIG_UBSAN_BOUNDS (for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family functions).
While there, use struct_size() helper, instead of the open-coded version, to calculate the size for the allocation of the whole flexible structure, including of course, the flexible-array member.

Quoted 5 lines

This code was found with the help of Coccinelle, and audited and fixed manually.
Signed-off-by Gustavo A. R. Silva <[email protected]>
Reviewed-by Kees Cook <[email protected]>

Patch applied to wireless-next.git, thanks.

5cf47dc14158 wifi: hostap: Add __counted_by for struct prism2_download_data and use struct_size()

Signature · 3 lines

https://patchwork.kernel.org/project/linux-wireless/patch/ZSRXXvWMMkm7qqRW@work/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches