NetBSD-Bugs archive
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]
kern/60034: Eliminate -Wformat-nonliteral warnings and improve security in ahd, ddb, and dev_verbose.c
>Number: 60034
>Category: kern
>Synopsis: Eliminate -Wformat-nonliteral warnings and improve security in ahd, ddb, and dev_verbose.c
>Confidential: no
>Severity: non-critical
>Priority: medium
>Responsible: kern-bug-people
>State: open
>Class: change-request
>Submitter-Id: net
>Arrival-Date: Tue Feb 24 22:55:00 +0000 2026
>Originator: João Bonifácio
>Release: NetBSD-current (11.99.5) as of February 2026
>Organization:
>Environment:
NetBSD 11.99.5 amd64 (Cross-compiled on Linux 6.18.12-100.fc42.x86_64)
>Description:
This PR addresses several -Wformat-nonliteral warnings across different
subsystems (pci, ddb, and dev_verbose). These warnings are currently
blocking the transition to a higher WARNS level (WARNS=5) for these files.
Summary of changes:
1. sys/dev/pci/ahd_pci.c: Refactored pci_status_strings and split_status_strings
to contain plain messages, passing them as "%s" arguments to printf.
2. sys/ddb/db_output.c: Refactored db_format_radix and db_format_hex to use
explicit snprintf format literals. Casted quad_t values to (unsigned long long)
to ensure format string compatibility.
3. sys/dev/dev_verbose.c: Replaced dynamic snprintf format strings with
explicit literal checks.
These changes improve kernel security by preventing potential format string
vulnerabilities and ensure the code is compatible with strict compiler checks.
>How-To-Repeat:
Compile the kernel with WARNS=5 or manually add -Wformat-nonliteral to
COPTS. The build will fail with -Werror in the mentioned files.
>Fix:
Apply the attached patches (0001 to 0003).
All patches have been tested for compilation on amd64 and preserve the
original dmesg output/formatting.
Patch 1: fix: sys/dev/pci/ahd_pci.c -Wformat-nonliteral errors
>From 6f8770ee46ab6c90e573ea3337a6920acb851ecb Mon Sep 17 00:00:00 2001
From: Joao Bonifacio <joaoboni017%gmail.com@localhost>
Date: Mon, 16 Feb 2026 11:03:48 -0300
Subject: [PATCH 1/3] fix: sys/dev/pci/ahd_pci.c -Wformat-nonliteral errors
Avoid potential security issues and compilation errors by ensuring
printf format strings are literals. Variable messages from
pci_status_strings and split_status_strings are now passed as
arguments (%s).
No functional change intended.
Signed-off-by: Joao Bonifacio <joaoboni017%gmail.com@localhost>
---
sys/dev/pci/ahd_pci.c | 54 +++++++++++++++++++++----------------------
1 file changed, 27 insertions(+), 27 deletions(-)
diff --git a/sys/dev/pci/ahd_pci.c b/sys/dev/pci/ahd_pci.c
index daba375a9..cebeaaa94 100644
--- a/sys/dev/pci/ahd_pci.c
+++ b/sys/dev/pci/ahd_pci.c
@@ -924,26 +924,26 @@ static const char *pci_status_source[] =
static const char *split_status_strings[] =
{
- "%s: Received split response in %s.\n",
- "%s: Received split completion error message in %s\n",
- "%s: Receive overrun in %s\n",
- "%s: Count not complete in %s\n",
- "%s: Split completion data bucket in %s\n",
- "%s: Split completion address error in %s\n",
- "%s: Split completion byte count error in %s\n",
- "%s: Signaled Target-abort to early terminate a split in %s\n"
+ "Received split response",
+ "Received split completion error message",
+ "Receive overrun",
+ "Count not complete",
+ "Split completion data bucket",
+ "Split completion address error",
+ "Split completion byte count error",
+ "Signaled Target-abort to early terminate a split"
};
static const char *pci_status_strings[] =
{
- "%s: Data Parity Error has been reported via PERR# in %s\n",
- "%s: Target initial wait state error in %s\n",
- "%s: Split completion read data parity error in %s\n",
- "%s: Split completion address attribute parity error in %s\n",
- "%s: Received a Target Abort in %s\n",
- "%s: Received a Master Abort in %s\n",
- "%s: Signal System Error Detected in %s\n",
- "%s: Address or Write Phase Parity Error Detected in %s.\n"
+ "Data Parity Error has been reported via PERR#",
+ "Target initial wait state error",
+ "Split completion read data parity error",
+ "Split completion address attribute parity error",
+ "Received a Target Abort",
+ "Received a Master Abort",
+ "Signal System Error Detected",
+ "Address or Write Phase Parity Error Detected"
};
static int
@@ -987,12 +987,12 @@ ahd_pci_intr(struct ahd_softc *ahd)
for (bit = 0; bit < 8; bit++) {
if ((pci_status[i] & (0x1 << bit)) != 0) {
- static const char *s;
+ static const char *msg;
- s = pci_status_strings[bit];
+ msg = pci_status_strings[bit];
if (i == 7/*TARG*/ && bit == 3)
- s = "%s: Signaled Target Abort\n";
- printf(s, ahd_name(ahd), pci_status_source[i]);
+ msg = "Signaled Target Abort\n";
+ printf("%s: %s in %s\n", ahd_name(ahd), msg, pci_status_source[i]);
}
}
}
@@ -1051,21 +1051,21 @@ ahd_pci_split_intr(struct ahd_softc *ahd, u_int intstat)
for (bit = 0; bit < 8; bit++) {
if ((split_status[i] & (0x1 << bit)) != 0) {
- static const char *s;
+ static const char *msg;
- s = split_status_strings[bit];
- printf(s, ahd_name(ahd),
- split_status_source[i]);
+ msg = split_status_strings[bit];
+ printf("%s: %s in %s\n",
+ ahd_name(ahd), msg, split_status_source[i]);
}
if (i > 0)
continue;
if ((sg_split_status[i] & (0x1 << bit)) != 0) {
- static const char *s;
+ static const char *msg;
- s = split_status_strings[bit];
- printf(s, ahd_name(ahd), "SG");
+ msg = split_status_strings[bit];
+ printf("%s: %s in %s\n", ahd_name(ahd), msg, "SG");
}
}
}
--
2.53.0
Patch 2: Fix format-nonliteral and type validation in db_output.c
>From 53ad467d9b2a430fa5d012baa34a6e2488c346c2 Mon Sep 17 00:00:00 2001
From: Joao Bonifacio <joaoboni017%gmail.com@localhost>
Date: Fri, 20 Feb 2026 23:45:49 -0300
Subject: [PATCH 2/3] sys/ddb: Fix format-nonliteral and type validation in
db_output.c
Refactor db_format_radix() and db_format_hex() to use explicit string
literals in snprintf() calls.
The previous implementation relied on dynamic pointer manipulation and
arithmetic to toggle the negative sign prefix. This approach bypasses
static analysis of the format string, leading to potential type safety
issues and build failures under strict compiler security flags.
This change replaces the dynamic logic with explicit conditional blocks
and static format strings. This enables proper validation of 'quad_t'
arguments against their respective conversion specifiers, ensuring
better code safety and compatibility with modern toolchains.
No functional changes intended.
Signed-off-by: Joao Bonifacio <joaoboni017%gmail.com@localhost>
---
sys/ddb/db_output.c | 50 ++++++++++++++++++++++++++-------------------
1 file changed, 29 insertions(+), 21 deletions(-)
diff --git a/sys/ddb/db_output.c b/sys/ddb/db_output.c
index a0662dbb2..03d111849 100644
--- a/sys/ddb/db_output.c
+++ b/sys/ddb/db_output.c
@@ -219,25 +219,27 @@ db_end_line(void)
void
db_format_radix(char *buf, size_t bufsiz, quad_t val, int altflag)
{
- const char *fmt;
if (db_radix == 16) {
db_format_hex(buf, bufsiz, val, altflag);
return;
}
- if (db_radix == 8)
- fmt = altflag ? "-%#qo" : "-%qo";
- else
- fmt = altflag ? "-%#qu" : "-%qu";
-
- if (val < 0)
- val = -val;
- else
- ++fmt;
-
- snprintf(buf, bufsiz, fmt, val);
-}
+ if (val < 0)
+ {
+ val = -val;
+ if (db_radix == 8)
+ snprintf(buf, bufsiz, altflag ? "-%#llo" : "-%llo", (unsigned long long)val);
+ else
+ snprintf(buf, bufsiz, "-%llu", (unsigned long long)val);
+ }
+ else {
+ if (db_radix == 8)
+ snprintf(buf, bufsiz, altflag ? "%#llo" : "%llo",(unsigned long long)val);
+ else
+ snprintf(buf, bufsiz, "%llu", (unsigned long long)val);
+ }
+ }
/*
* Replacement for old '%z' kprintf format.
@@ -245,15 +247,21 @@ db_format_radix(char *buf, size_t bufsiz, quad_t val, int altflag)
void
db_format_hex(char *buf, size_t bufsiz, quad_t val, int altflag)
{
- /* Only use alternate form if val is nonzero. */
- const char *fmt = (altflag && val) ? "-%#qx" : "-%qx";
-
- if (val < 0)
+ /*Negative number*/
+ if (val < 0){
val = -val;
- else
- ++fmt;
-
- snprintf(buf, bufsiz, fmt, val);
+ if (altflag)
+ snprintf(buf, bufsiz, "-%#llx", (unsigned long long)val);
+ else
+ snprintf(buf, bufsiz, "-%llx", (unsigned long long)val);
+ }
+ /*Positive number*/
+ else{
+ if (altflag && val)
+ snprintf(buf, bufsiz, "%#llx",(unsigned long long)val);
+ else
+ snprintf(buf, bufsiz, "%llx", (unsigned long long)val);
+ }
}
/*
--
2.53.0
Patch 3: Fix -Wformat-nonliteral and improve security in dev_verbose.c
>From e0770ec54298276f07c27b06a89a56dc853a0cc5 Mon Sep 17 00:00:00 2001
From: Joao Bonifacio <joaoboni017%gmail.com@localhost>
Date: Sat, 21 Feb 2026 17:32:52 -0300
Subject: [PATCH 3/3] sys/dev: Fix -Wformat-nonliteral and improve security in
dev_verbose.c
This patch replaces the use of a dynamic format string with static literals in dev_findvendor and dev_findproduct.
By using fixed strings like '0x%04x', we ensure that the compiler can verify the arguments at build time. More importantly, this makes the code immune to 'format string attacks'. Previously, a buggy driver or malicious hardware could theoretically pass a crafted format string (like '%s%s%s') to trigger a kernel panic or leak sensitive memory. Now, the format is controlled by the kernel, not by the input data.
Tested on amd64: compiles cleanly with -Werror.
Signed-off-by: Joao Bonifacio <joaoboni017%gmail.com@localhost>
---
sys/dev/dev_verbose.c | 21 +++++++++++++++++++--
1 file changed, 19 insertions(+), 2 deletions(-)
diff --git a/sys/dev/dev_verbose.c b/sys/dev/dev_verbose.c
index 5c46ed425..9df5300d1 100644
--- a/sys/dev/dev_verbose.c
+++ b/sys/dev/dev_verbose.c
@@ -73,7 +73,16 @@ dev_findvendor(char *buf, size_t len, const char *words, size_t nwords,
while (n < nvendors && vendors[n] != 0)
n++;
}
- snprintf(buf, len, fmt, vendor);
+
+ /*
+ * Avoid -Wformat-nonliteral:
+ */
+ if (fmt != NULL && strcmp(fmt, "0x%04x") == 0){
+ snprintf(buf, len, "0x%04x", vendor);
+ } else {
+ snprintf(buf, len, "0x%08x", vendor);
+ }
+
return NULL;
}
@@ -94,6 +103,14 @@ dev_findproduct(char *buf, size_t len, const char *words, size_t nwords,
while (n < nproducts && products[n] != 0)
n++;
}
- snprintf(buf, len, fmt, product);
+ /*
+ * Avoid Wformat-nonliteral
+ */
+ if (fmt != NULL && strcmp(fmt, "0x%04x") == 0){
+ snprintf(buf, len, "0x%04x", product);
+ } else {
+ snprintf(buf, len, "0x%08x", product);
+ }
+
return NULL;
}
--
2.53.0
Similar fixes are needed for ixgbe(4), which I am still investigating.
Home |
Main Index |
Thread Index |
Old Index