Re: [PATCH][next] wifi: rtw89: coex: Fix -Wstringop-overflow warnings in _append_tdma()
"Gustavo A. R. Silva" writes: >> wireless-next has taken my patch [1] that is identical to yours. > > Great! > > I had mine ready on Oct 31, but I was waiting for the merge window to close > before sending it. BTW we keep wireless-next open also during merge windows. So no need to hold up wireless patches because of the merge window. I know it's confusing that we do differently than how net-next works but this is easier for us, less patch build up. -- https://patchwork.kernel.org/project/linux-wireless/list/ https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Re: [PATCH] qnx4: fix to avoid panic due to buffer overflow
On Tue, Nov 14, 2023 at 1:40 AM Anders Larsen wrote: > < Snipped> > > sizeof(de->di_fname) is evaluated as QNX4_SHORT_NAME_MAX already at compile > time, see the definition of di_fname in uapi/linux/qnx4_fs.h > > I agree that the code is confusing, as 'de' is declared as a pointer to a > struct qnx4_inode_entry but in reality points to a struct qnx4_link_info iff > QNX4_FILE_LINK is set in de->di_status. > (Note that the corresponding field dl_status in qnx4_link_info is at the same > offset as di_status in qnx4_inode_entry - that's the disk layout.) > Thanks for the details, yes in struct qnx4_inode_entry its size char di_fname[QNX4_SHORT_NAME_MAX]; < snipped> > > Niek reported that this fix improved the situation, but he later got a crash, > albeit at a different place (but still within the qnx4fs). Yes I saw that Niek has shared the second crash dump stack in above email thread and also in [1] Bugzilla 218111.The dump stack of the crash looks to be doing a similar lookup call context, do_statx => vfs_statx => filename_lookup => qn4x_lookup => fortify_panic ( ) [1] https://bugzilla.kernel.org/show_bug.cgi?id=218111#c4 But I also see a softlockup also in the dump stack, so something in their environment is causing softlock ups. And that tallies with the symptoms of system freeze that Niek mentioned " I can mount and view the directories, but after several hours my system froze up again." watchdog: BUG: soft lockup - CPU#7 stuck for 26s! [pool-gvfsd-admi:31952] It's possible the softlockups were occurring on fews of the CPUs on the system for a few hours before the crash occurred that caused a system slow down. BR, Ronald
Re: [PATCH][next] hwmon: (aspeed-pwm-tacho) Fix -Wstringop-overflow warning in aspeed_create_fan_tach_channel()
On Mon, Nov 13, 2023 at 01:38:12PM -0600, Gustavo A. R. Silva wrote:
> Based on the documentation below, the maximum number of Fan tach
> channels is 16:
>
> Documentation/devicetree/bindings/hwmon/aspeed-pwm-tacho.txt:45:
> 45 - aspeed,fan-tach-ch : should specify the Fan tach input channel.
> 46 integer value in the range 0 through 15, with 0 indicating
> 47 Fan tach channel 0 and 15 indicating Fan tach channel 15.
> 48 At least one Fan tach input channel is required.
>
> However, the compiler doesn't know that, and legitimaly warns about a
> potential
> overwrite in array `u8 fan_tach_ch_source[16]` in `struct
> aspeed_pwm_tacho_data`,
> in case `index` takes a value outside the boundaries of the array:
>
All that doesn't warrant introducing checkpatch warnings.
> drivers/hwmon/aspeed-pwm-tacho.c:
> 179 struct aspeed_pwm_tacho_data {
> ...
> 184 bool fan_tach_present[16];
> ...
> 193 u8 fan_tach_ch_source[16];
> ...
> 196 };
>
> In function ‘aspeed_create_fan_tach_channel’,
> inlined from ‘aspeed_create_fan’ at
> drivers/hwmon/aspeed-pwm-tacho.c:877:2,
> inlined from ‘aspeed_pwm_tacho_probe’ at
> drivers/hwmon/aspeed-pwm-tacho.c:936:9:
> drivers/hwmon/aspeed-pwm-tacho.c:751:49: warning: writing 1 byte into a
> region of size 0 [-Wstringop-overflow=]
> 751 | priv->fan_tach_ch_source[index] = pwm_source;
> | ^~~~
> drivers/hwmon/aspeed-pwm-tacho.c: In function ‘aspeed_pwm_tacho_probe’:
> drivers/hwmon/aspeed-pwm-tacho.c:193:12: note: at offset [48, 255] into
> destination object ‘fan_tach_ch_source’ of size 16
> 193 | u8 fan_tach_ch_source[16];
> |^~
>
> Fix this by sanity checking `index` before using it to index arrays of
> size 16 elements in `struct aspeed_pwm_tacho_data`. Also, and just for
> completeness, add a `pr_err()` message to display in the unlikely case
> `0 > index >= 16`.
>
> This is probably the last remaining -Wstringop-overflow issue in the
> kernel, and this patch helps with the ongoing efforts to enable such
> compiler option globally.
>
I am sorry, but this description almost completely misses the point.
The real issue is that the values in aspeed,fan-tach-ch are not range
checked, which can cause real problems if is elements are set to values
larger than 15. This is a real problem and has nothing to do with string
overflows.
> Signed-off-by: Gustavo A. R. Silva
> ---
> drivers/hwmon/aspeed-pwm-tacho.c | 18 --
> 1 file changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/hwmon/aspeed-pwm-tacho.c
> b/drivers/hwmon/aspeed-pwm-tacho.c
> index 997df4b40509..092a81916325 100644
> --- a/drivers/hwmon/aspeed-pwm-tacho.c
> +++ b/drivers/hwmon/aspeed-pwm-tacho.c
> @@ -166,6 +166,8 @@
>
> #define MAX_CDEV_NAME_LEN 16
>
> +#define MAX_ASPEED_FAN_TACH_CHANNELS 16
> +
> struct aspeed_cooling_device {
> char name[16];
> struct aspeed_pwm_tacho_data *priv;
> @@ -181,7 +183,7 @@ struct aspeed_pwm_tacho_data {
> struct reset_control *rst;
> unsigned long clk_freq;
> bool pwm_present[8];
> - bool fan_tach_present[16];
> + bool fan_tach_present[MAX_ASPEED_FAN_TACH_CHANNELS];
> u8 type_pwm_clock_unit[3];
> u8 type_pwm_clock_division_h[3];
> u8 type_pwm_clock_division_l[3];
> @@ -190,7 +192,7 @@ struct aspeed_pwm_tacho_data {
> u16 type_fan_tach_unit[3];
> u8 pwm_port_type[8];
> u8 pwm_port_fan_ctrl[8];
> - u8 fan_tach_ch_source[16];
> + u8 fan_tach_ch_source[MAX_ASPEED_FAN_TACH_CHANNELS];
> struct aspeed_cooling_device *cdev[8];
> const struct attribute_group *groups[3];
> };
> @@ -746,10 +748,14 @@ static void aspeed_create_fan_tach_channel(struct
> aspeed_pwm_tacho_data *priv,
>
> for (val = 0; val < count; val++) {
> index = fan_tach_ch[val];
> - aspeed_set_fan_tach_ch_enable(priv->regmap, index, true);
> - priv->fan_tach_present[index] = true;
> - priv->fan_tach_ch_source[index] = pwm_source;
> - aspeed_set_fan_tach_ch_source(priv->regmap, index, pwm_source);
> + if (index < MAX_ASPEED_FAN_TACH_CHANNELS) {
> + aspeed_set_fan_tach_ch_enable(priv->regmap, index,
> true);
> + priv->fan_tach_present[index] = true;
> + priv->fan_tach_ch_source[index] = pwm_source;
> + aspeed_set_fan_tach_ch_source(priv->regmap, index,
> pwm_source);
> + } else {
> + pr_err("Invalid Fan Tach input channel %u\n.", index);
This should use dev_err() (and, yes, that means dev needs to be passed
as argument), and the function should return -EINVAL if this is
encountered. Also, error handling should come first.
if (index >= MAX_ASPEED_FAN_TACH_CHANNELS) {
[GIT PULL] hardening fixes for v6.7-rc2
Hi Linus, Please pull these small hardening fixes for v6.7-rc2. Thanks! -Kees The following changes since commit 9cca73d7b4bfec75b2fcef751015f31691afa792: hwmon: (acpi_power_meter) replace open-coded kmemdup_nul (2023-10-24 14:10:53 -0700) are available in the Git repository at: https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git tags/hardening-v6.7-rc2 for you to fetch changes up to 782ce431613cf08c3a00dca42ad925c3b1108d09: gcc-plugins: latent_entropy: Fix typo (args -> argc) in plugin description (2023-11-14 09:32:47 -0800) kernel hardening fixes for v6.7-rc2 - stackleak: add declarations for global functions (Arnd Bergmann) - gcc-plugins: randstruct: Only warn about true flexible arrays (Kees Cook) - gcc-plugins: latent_entropy: Fix description typo (Konstantin Runov) Arnd Bergmann (1): stackleak: add declarations for global functions Kees Cook (1): gcc-plugins: randstruct: Only warn about true flexible arrays Konstantin Runov (1): gcc-plugins: latent_entropy: Fix typo (args -> argc) in plugin description include/linux/stackleak.h | 6 ++ scripts/gcc-plugins/latent_entropy_plugin.c | 4 ++-- scripts/gcc-plugins/randomize_layout_plugin.c | 10 -- 3 files changed, 8 insertions(+), 12 deletions(-) -- Kees Cook
[PATCH] SUNRPC: Replace strlcpy() with strscpy()
strlcpy() reads the entire source buffer first. This read may exceed
the destination size limit. This is both inefficient and can lead
to linear read overflows if a source string is not NUL-terminated[1].
Additionally, it returns the size of the source string, not the
resulting size of the destination string. In an effort to remove strlcpy()
completely[2], replace strlcpy() here with strscpy().
Explicitly handle the truncation case by returning the size of the
resulting string.
If "nodename" was ever longer than sizeof(clnt->cl_nodename) - 1, this
change will fix a bug where clnt->cl_nodelen would end up thinking there
were more characters in clnt->cl_nodename than there actually were,
which might have lead to kernel memory content exposures.
Cc: Trond Myklebust
Cc: Anna Schumaker
Cc: Chuck Lever
Cc: Jeff Layton
Cc: Neil Brown
Cc: Olga Kornievskaia
Cc: Dai Ngo
Cc: Tom Talpey
Cc: "David S. Miller"
Cc: Eric Dumazet
Cc: Jakub Kicinski
Cc: Paolo Abeni
Cc: linux-...@vger.kernel.org
Cc: net...@vger.kernel.org
Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy [1]
Link: https://github.com/KSPP/linux/issues/89 [2]
Co-developed-by: Azeem Shaikh
Signed-off-by: Azeem Shaikh
Signed-off-by: Kees Cook
---
net/sunrpc/clnt.c | 10 --
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
index daa9582ec861..7afe02bdea4a 100644
--- a/net/sunrpc/clnt.c
+++ b/net/sunrpc/clnt.c
@@ -287,8 +287,14 @@ static struct rpc_xprt *rpc_clnt_set_transport(struct
rpc_clnt *clnt,
static void rpc_clnt_set_nodename(struct rpc_clnt *clnt, const char *nodename)
{
- clnt->cl_nodelen = strlcpy(clnt->cl_nodename,
- nodename, sizeof(clnt->cl_nodename));
+ ssize_t copied;
+
+ copied = strscpy(clnt->cl_nodename,
+nodename, sizeof(clnt->cl_nodename));
+
+ clnt->cl_nodelen = copied < 0
+ ? sizeof(clnt->cl_nodename) - 1
+ : copied;
}
static int rpc_client_register(struct rpc_clnt *clnt,
--
2.34.1
[PATCH v2][next] hwmon: (aspeed-pwm-tacho) Fix -Wstringop-overflow warning in aspeed_create_fan_tach_channel()
Based on the documentation below, the maximum number of Fan tach
channels is 16:
Documentation/devicetree/bindings/hwmon/aspeed-pwm-tacho.txt:45:
45 - aspeed,fan-tach-ch : should specify the Fan tach input channel.
46 integer value in the range 0 through 15, with 0 indicating
47 Fan tach channel 0 and 15 indicating Fan tach channel 15.
48 At least one Fan tach input channel is required.
However, the compiler doesn't know that, and legitimaly warns about a potential
overwrite in array `u8 fan_tach_ch_source[16]` in `struct
aspeed_pwm_tacho_data`,
in case `index` takes a value outside the boundaries of the array:
drivers/hwmon/aspeed-pwm-tacho.c:
179 struct aspeed_pwm_tacho_data {
...
184 bool fan_tach_present[16];
...
193 u8 fan_tach_ch_source[16];
196 };
In function ‘aspeed_create_fan_tach_channel’,
inlined from ‘aspeed_create_fan’ at drivers/hwmon/aspeed-pwm-tacho.c:877:2,
inlined from ‘aspeed_pwm_tacho_probe’ at
drivers/hwmon/aspeed-pwm-tacho.c:936:9:
drivers/hwmon/aspeed-pwm-tacho.c:751:49: warning: writing 1 byte into a region
of size 0 [-Wstringop-overflow=]
751 | priv->fan_tach_ch_source[index] = pwm_source;
| ^~~~
drivers/hwmon/aspeed-pwm-tacho.c: In function ‘aspeed_pwm_tacho_probe’:
drivers/hwmon/aspeed-pwm-tacho.c:193:12: note: at offset [48, 255] into
destination object ‘fan_tach_ch_source’ of size 16
193 | u8 fan_tach_ch_source[16];
|^~
Fix this by sanity checking `index` before using it to index arrays of
size 16 elements in `struct aspeed_pwm_tacho_data`. Also, pass `dev` as
argument to function `aspeed_create_fan_tach_channel()`, and add an error
message in case `index` is out-of-bounds, in which case return `-EINVAL`.
Signed-off-by: Gustavo A. R. Silva
---
Changes in v2:
- Pass `dev` to function aspeed_create_fan_tach_channel() and return
-EINVAL in case of error. (Guenter)
- Update changelog text.
v1:
- Link: https://lore.kernel.org/linux-hardening/ZVJ7JBFoULzY3VGx@work/
drivers/hwmon/aspeed-pwm-tacho.c | 20
1 file changed, 16 insertions(+), 4 deletions(-)
diff --git a/drivers/hwmon/aspeed-pwm-tacho.c b/drivers/hwmon/aspeed-pwm-tacho.c
index 997df4b40509..9a209e064e46 100644
--- a/drivers/hwmon/aspeed-pwm-tacho.c
+++ b/drivers/hwmon/aspeed-pwm-tacho.c
@@ -166,6 +166,8 @@
#define MAX_CDEV_NAME_LEN 16
+#define MAX_ASPEED_FAN_TACH_CHANNELS 16
+
struct aspeed_cooling_device {
char name[16];
struct aspeed_pwm_tacho_data *priv;
@@ -181,7 +183,7 @@ struct aspeed_pwm_tacho_data {
struct reset_control *rst;
unsigned long clk_freq;
bool pwm_present[8];
- bool fan_tach_present[16];
+ bool fan_tach_present[MAX_ASPEED_FAN_TACH_CHANNELS];
u8 type_pwm_clock_unit[3];
u8 type_pwm_clock_division_h[3];
u8 type_pwm_clock_division_l[3];
@@ -190,7 +192,7 @@ struct aspeed_pwm_tacho_data {
u16 type_fan_tach_unit[3];
u8 pwm_port_type[8];
u8 pwm_port_fan_ctrl[8];
- u8 fan_tach_ch_source[16];
+ u8 fan_tach_ch_source[MAX_ASPEED_FAN_TACH_CHANNELS];
struct aspeed_cooling_device *cdev[8];
const struct attribute_group *groups[3];
};
@@ -737,7 +739,8 @@ static void aspeed_create_pwm_port(struct
aspeed_pwm_tacho_data *priv,
aspeed_set_pwm_port_fan_ctrl(priv, pwm_port, INIT_FAN_CTRL);
}
-static void aspeed_create_fan_tach_channel(struct aspeed_pwm_tacho_data *priv,
+static int aspeed_create_fan_tach_channel(struct device *dev,
+ struct aspeed_pwm_tacho_data *priv,
u8 *fan_tach_ch,
int count,
u8 pwm_source)
@@ -746,11 +749,17 @@ static void aspeed_create_fan_tach_channel(struct
aspeed_pwm_tacho_data *priv,
for (val = 0; val < count; val++) {
index = fan_tach_ch[val];
+ if (index >= MAX_ASPEED_FAN_TACH_CHANNELS) {
+ dev_err(dev, "Invalid Fan Tach input channel %u\n.",
index);
+ return -EINVAL;
+ }
aspeed_set_fan_tach_ch_enable(priv->regmap, index, true);
priv->fan_tach_present[index] = true;
priv->fan_tach_ch_source[index] = pwm_source;
aspeed_set_fan_tach_ch_source(priv->regmap, index, pwm_source);
}
+
+ return 0;
}
static int
@@ -874,7 +883,10 @@ static int aspeed_create_fan(struct device *dev,
fan_tach_ch, count);
if (ret)
return ret;
- aspeed_create_fan_tach_channel(priv, fan_tach_ch, count, pwm_port);
+
+ ret = aspeed_create_fan_tach_channel(dev, priv, fan_tach_ch, count,
pwm_port);
+ if (ret)
+ re
Re: [PATCH][next] hwmon: (aspeed-pwm-tacho) Fix -Wstringop-overflow warning in aspeed_create_fan_tach_channel()
On 11/14/23 08:52, Guenter Roeck wrote:
On Mon, Nov 13, 2023 at 01:38:12PM -0600, Gustavo A. R. Silva wrote:
Based on the documentation below, the maximum number of Fan tach
channels is 16:
Documentation/devicetree/bindings/hwmon/aspeed-pwm-tacho.txt:45:
45 - aspeed,fan-tach-ch : should specify the Fan tach input channel.
46 integer value in the range 0 through 15, with 0 indicating
47 Fan tach channel 0 and 15 indicating Fan tach channel 15.
48 At least one Fan tach input channel is required.
However, the compiler doesn't know that, and legitimaly warns about a potential
overwrite in array `u8 fan_tach_ch_source[16]` in `struct
aspeed_pwm_tacho_data`,
in case `index` takes a value outside the boundaries of the array:
All that doesn't warrant introducing checkpatch warnings.
Do you mean this?
WARNING: Prefer a maximum 75 chars per line (possible unwrapped commit
description?)
#17:
46 integer value in the range 0 through 15, with 0 indicating
I honestly didn't consider that relevant, and I didn't want to alter the format
of
the Doc text.
However, if you want me to split any offending line, that's not a problem. Just
let
me know. :)
drivers/hwmon/aspeed-pwm-tacho.c:
179 struct aspeed_pwm_tacho_data {
...
184 bool fan_tach_present[16];
...
193 u8 fan_tach_ch_source[16];
...
196 };
In function ‘aspeed_create_fan_tach_channel’,
inlined from ‘aspeed_create_fan’ at drivers/hwmon/aspeed-pwm-tacho.c:877:2,
inlined from ‘aspeed_pwm_tacho_probe’ at
drivers/hwmon/aspeed-pwm-tacho.c:936:9:
drivers/hwmon/aspeed-pwm-tacho.c:751:49: warning: writing 1 byte into a region
of size 0 [-Wstringop-overflow=]
751 | priv->fan_tach_ch_source[index] = pwm_source;
| ^~~~
drivers/hwmon/aspeed-pwm-tacho.c: In function ‘aspeed_pwm_tacho_probe’:
drivers/hwmon/aspeed-pwm-tacho.c:193:12: note: at offset [48, 255] into
destination object ‘fan_tach_ch_source’ of size 16
193 | u8 fan_tach_ch_source[16];
|^~
Fix this by sanity checking `index` before using it to index arrays of
size 16 elements in `struct aspeed_pwm_tacho_data`. Also, and just for
completeness, add a `pr_err()` message to display in the unlikely case
`0 > index >= 16`.
This is probably the last remaining -Wstringop-overflow issue in the
kernel, and this patch helps with the ongoing efforts to enable such
compiler option globally.
I am sorry, but this description almost completely misses the point.
The real issue is that the values in aspeed,fan-tach-ch are not range
checked, which can cause real problems if is elements are set to values
larger than 15. This is a real problem and has nothing to do with string
overflows.
Yeah, the above paragraph was extra, and I removed it in v2[1]. The rest
of the changelog text describes the issue in the code.
This should use dev_err() (and, yes, that means dev needs to be passed
as argument), and the function should return -EINVAL if this is
encountered. Also, error handling should come first.
if (index >= MAX_ASPEED_FAN_TACH_CHANNELS) {
dev_err(dev, "Invalid Fan Tach input channel %u\n.",
index);
return -EINVAL;
}
Done in v2.
Thanks a lot for the feedback.
--
Gustavo
[1] https://lore.kernel.org/linux-hardening/ZVPQJIP26dIzRAr6@work/
Re: [PATCH v2][next] hwmon: (aspeed-pwm-tacho) Fix -Wstringop-overflow warning in aspeed_create_fan_tach_channel()
On Tue, Nov 14, 2023 at 01:53:08PM -0600, Gustavo A. R. Silva wrote:
> Based on the documentation below, the maximum number of Fan tach
> channels is 16:
>
> Documentation/devicetree/bindings/hwmon/aspeed-pwm-tacho.txt:45:
> 45 - aspeed,fan-tach-ch : should specify the Fan tach input channel.
> 46 integer value in the range 0 through 15, with 0 indicating
> 47 Fan tach channel 0 and 15 indicating Fan tach channel 15.
> 48 At least one Fan tach input channel is required.
>
> However, the compiler doesn't know that, and legitimaly warns about a
> potential
> overwrite in array `u8 fan_tach_ch_source[16]` in `struct
> aspeed_pwm_tacho_data`,
> in case `index` takes a value outside the boundaries of the array:
>
> drivers/hwmon/aspeed-pwm-tacho.c:
> 179 struct aspeed_pwm_tacho_data {
> ...
> 184 bool fan_tach_present[16];
> ...
> 193 u8 fan_tach_ch_source[16];
> 196 };
>
> In function ‘aspeed_create_fan_tach_channel’,
> inlined from ‘aspeed_create_fan’ at
> drivers/hwmon/aspeed-pwm-tacho.c:877:2,
> inlined from ‘aspeed_pwm_tacho_probe’ at
> drivers/hwmon/aspeed-pwm-tacho.c:936:9:
> drivers/hwmon/aspeed-pwm-tacho.c:751:49: warning: writing 1 byte into a
> region of size 0 [-Wstringop-overflow=]
> 751 | priv->fan_tach_ch_source[index] = pwm_source;
> | ^~~~
> drivers/hwmon/aspeed-pwm-tacho.c: In function ‘aspeed_pwm_tacho_probe’:
> drivers/hwmon/aspeed-pwm-tacho.c:193:12: note: at offset [48, 255] into
> destination object ‘fan_tach_ch_source’ of size 16
> 193 | u8 fan_tach_ch_source[16];
> |^~
>
> Fix this by sanity checking `index` before using it to index arrays of
> size 16 elements in `struct aspeed_pwm_tacho_data`. Also, pass `dev` as
> argument to function `aspeed_create_fan_tach_channel()`, and add an error
> message in case `index` is out-of-bounds, in which case return `-EINVAL`.
>
> Signed-off-by: Gustavo A. R. Silva
Thanks for the v2! This looks good; it's able to pass back the error
now.
Reviewed-by: Kees Cook
--
Kees Cook
Re: [GIT PULL] hardening fixes for v6.7-rc2
The pull request you sent on Tue, 14 Nov 2023 09:41:28 -0800: > https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git > tags/hardening-v6.7-rc2 has been merged into torvalds/linux.git: https://git.kernel.org/torvalds/c/c42d9eeef8e5ba9292eda36fd8e3c11f35ee065c Thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/prtracker.html
