Re: the nul-terminated string helper desk chair rearrangement
On Fri, Oct 20, 2023 at 11:30:49AM -0700, Kees Cook wrote: > I'm curious where you looked and didn't find documentation -- perhaps > there is an improvement to be made to aim one to where the existing > documentation lives? My order was the following: - look for kernel doc on the main function implementation in lib/string.c (as found by a grep for an EXPORT_SYMBOL for it) - after not finding it there, but seeing that it has an ifdef for an arch override, which turns out to be unused - then I grepped the Documentation/ directory for it, and while there are quite a few matches for strscpy, they are largely in examples, with the only text referring to strscpy being Documentation/process/deprecated.rst that tells you to use it instead of strcpy, but not how it actually works - after that I realized that some people put the kerneldoc on the declaration, so I looked at that in string.h, but couldn't find it. > > There's some docs at [1]. Perhaps there could be more? > > > > [1]: > > https://elixir.bootlin.com/linux/v6.6-rc6/source/include/linux/fortify-string.h#L292 > > Right, And it's even valid kern-doc, which gets rendered in the kernel > API docs, along with all the other string functions: > https://docs.kernel.org/core-api/kernel-api.html#c.strscpy Well, I never use the generated kerneldoc because it's much harder than just grepping the tree, but indeed it exists even if it's hidden in the most obsfucated way. But at least I know now!
Re: the nul-terminated string helper desk chair rearrangement
On Thu, 2023-10-26 at 12:01 +0200, Christoph Hellwig wrote: > > > There's some docs at [1]. Perhaps there could be more? > > > > > > [1]: > > > https://elixir.bootlin.com/linux/v6.6-rc6/source/include/linux/fortify-string.h#L292 > > > > Right, And it's even valid kern-doc, which gets rendered in the > > kernel API docs, along with all the other string functions: > > https://docs.kernel.org/core-api/kernel-api.html#c.strscpy > > Well, I never use the generated kerneldoc because it's much harder > than just grepping the tree, but indeed it exists even if it's hidden > in the most obsfucated way. But at least I know now! This, honestly, is one of the really annoying problems of kerneldoc. When looking for structures or functions git grep " -" usually finds it. However, I recently asked on linux-scsi if we could point to the doc about system_state and what it meant. However, either we all suck or there's no such documentation because no-one could find it. While it's nice in theory to have everything documented, it's not much use if no one can actually find the information ... James
Re: the nul-terminated string helper desk chair rearrangement
> > > [1]: > > > https://elixir.bootlin.com/linux/v6.6-rc6/source/include/linux/fortify-string.h#L292 I found that https://elixir.bootlin.com/linux is the best way to find Documentation for functions and structures. I would suggest try it first, and only when what fails to start using grep. Andrew
Re: the nul-terminated string helper desk chair rearrangement
On Thu, Oct 26, 2023 at 03:44:22PM +0200, Andrew Lunn wrote: > > > > [1]: > > > > https://elixir.bootlin.com/linux/v6.6-rc6/source/include/linux/fortify-string.h#L292 > > I found that https://elixir.bootlin.com/linux is the best way to find > Documentation for functions and structures. I would suggest try it > first, and only when what fails to start using grep. It's painful to have to open the HTML documentation generated by the kernel build system when developing, and even more painful to have to use a particular website. If the documentation is difficult to locate in the source tree, we're doing something wrong. -- Regards, Laurent Pinchart
Re: the nul-terminated string helper desk chair rearrangement
On Thu, 26 Oct 2023 07:39:44 -0400 James Bottomley wrote: > While it's nice in theory to have everything documented, it's not much > use if no one can actually find the information ... Does kerneldoc provide an automated index? That is, if we had a single file that had every function in the kernel that is documented, with the path to the file that documents it, it would make finding documentation much simpler. Maybe it already does? Which would mean we need a way to find the index too! -- Steve
Re: the nul-terminated string helper desk chair rearrangement
Hi Steven,
On Thu, Oct 26, 2023 at 3:52 PM Steven Rostedt wrote:
> On Thu, 26 Oct 2023 07:39:44 -0400
> James Bottomley wrote:
>
> > While it's nice in theory to have everything documented, it's not much
> > use if no one can actually find the information ...
>
> Does kerneldoc provide an automated index? That is, if we had a single file
> that had every function in the kernel that is documented, with the path to
> the file that documents it, it would make finding documentation much
> simpler.
>
> Maybe it already does? Which would mean we need a way to find the index too!
ctags?
Although "git grep" is faster (assumed you use the "correct" search
pattern, which can sometimes be challenging, indeed).
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: the nul-terminated string helper desk chair rearrangement
Steven Rostedt writes: > On Thu, 26 Oct 2023 07:39:44 -0400 > James Bottomley wrote: > >> While it's nice in theory to have everything documented, it's not much >> use if no one can actually find the information ... > > Does kerneldoc provide an automated index? If you go to https://www.kernel.org/doc/html/latest/ and type a symbol into the search box on the left, you'll get directed to the right place (if it exists). For James's system_state example, it makes it clear that there's only one reference - in the coding-style document, of all places... I've never looked into that index to see how hard it would be to access without a browser. jon
Re: the nul-terminated string helper desk chair rearrangement
On Thu, 2023-10-26 at 15:44 +0200, Andrew Lunn wrote: > > > > [1]: > > > > https://elixir.bootlin.com/linux/v6.6-rc6/source/include/linux/fortify-string.h#L292 > > I found that https://elixir.bootlin.com/linux That's a 404, I think you mean https://elixir.bootlin.com/linux/latest/source > is the best way to find Documentation for functions and structures. > I would suggest try it first, and only when what fails to start using > grep. I just tried it with system_state and it doesn't even find the definition. I think it might be because it has annotations which confuse the searcher (it's in init/main.c as enum system_states system_state __read_mostly; ). If there's any meaningful doc about it, elixir also doesn't find it. James
[PATCH] seq_buf: Introduce DECLARE_SEQ_BUF and seq_buf_cstr()
Solve two ergonomic issues with struct seq_buf:
1) Too much boilerplate is required to initialize:
struct seq_buf s;
char buf[32];
seq_buf_init(s, buf, sizeof(buf));
Instead, we can build this directly on the stack. Provide
DECLARE_SEQ_BUF() macro to do this:
DECLARE_SEQ_BUF(s, 32);
2) %NUL termination is fragile and requires 2 steps to get a valid
C String (and is a layering violation exposing the "internals" of
seq_buf):
seq_buf_terminate(s);
do_something(s->buffer);
Instead, we can just return s->buffer direction after terminating it
in refactored seq_buf_terminate(), now known as seq_buf_cstr():
do_soemthing(seq_buf_cstr(s));
Cc: Steven Rostedt
Cc: "Matthew Wilcox (Oracle)"
Cc: Christoph Hellwig
Cc: Justin Stitt
Cc: Kent Overstreet
Cc: Petr Mladek
Cc: Andy Shevchenko
Cc: Rasmus Villemoes
Cc: Sergey Senozhatsky
Cc: Masami Hiramatsu
Cc: Greg Kroah-Hartman
Cc: Arnd Bergmann
Cc: Jonathan Corbet
Cc: Yun Zhou
Cc: Jacob Keller
Cc: Zhen Lei
Cc: linux-trace-ker...@vger.kernel.org
Signed-off-by: Kees Cook
---
include/linux/seq_buf.h | 19 +++
kernel/trace/trace.c| 11 +--
lib/seq_buf.c | 4 +---
3 files changed, 17 insertions(+), 17 deletions(-)
diff --git a/include/linux/seq_buf.h b/include/linux/seq_buf.h
index 8483e4b2d0d2..8896b830eb3d 100644
--- a/include/linux/seq_buf.h
+++ b/include/linux/seq_buf.h
@@ -21,9 +21,16 @@ struct seq_buf {
size_t len;
};
+#define DECLARE_SEQ_BUF(NAME, SIZE)\
+ char __ ## NAME ## _buffer[SIZE] = ""; \
+ struct seq_buf NAME = { .buffer = &__ ## NAME ## _buffer, \
+ .size = SIZE }
+
static inline void seq_buf_clear(struct seq_buf *s)
{
s->len = 0;
+ if (s->size)
+ s->buffer[0] = '\0';
}
static inline void
@@ -69,8 +76,8 @@ static inline unsigned int seq_buf_used(struct seq_buf *s)
}
/**
- * seq_buf_terminate - Make sure buffer is nul terminated
- * @s: the seq_buf descriptor to terminate.
+ * seq_buf_cstr - get %NUL-terminated C string from seq_buf
+ * @s: the seq_buf handle
*
* This makes sure that the buffer in @s is nul terminated and
* safe to read as a string.
@@ -81,16 +88,20 @@ static inline unsigned int seq_buf_used(struct seq_buf *s)
*
* After this function is called, s->buffer is safe to use
* in string operations.
+ *
+ * Returns @s->buf after making sure it is terminated.
*/
-static inline void seq_buf_terminate(struct seq_buf *s)
+static inline char *seq_buf_cstr(struct seq_buf *s)
{
if (WARN_ON(s->size == 0))
- return;
+ return "";
if (seq_buf_buffer_left(s))
s->buffer[s->len] = 0;
else
s->buffer[s->size - 1] = 0;
+
+ return s->buffer;
}
/**
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index d629065c2383..d83f36dc4bf8 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -3828,15 +3828,6 @@ static bool trace_safe_str(struct trace_iterator *iter,
const char *str,
return false;
}
-static const char *show_buffer(struct trace_seq *s)
-{
- struct seq_buf *seq = &s->seq;
-
- seq_buf_terminate(seq);
-
- return seq->buffer;
-}
-
static DEFINE_STATIC_KEY_FALSE(trace_no_verify);
static int test_can_verify_check(const char *fmt, ...)
@@ -3976,7 +3967,7 @@ void trace_check_vprintf(struct trace_iterator *iter,
const char *fmt,
*/
if (WARN_ONCE(!trace_safe_str(iter, str, star, len),
"fmt: '%s' current_buffer: '%s'",
- fmt, show_buffer(&iter->seq))) {
+ fmt, seq_buf_cstr(&iter->seq.seq))) {
int ret;
/* Try to safely read the string */
diff --git a/lib/seq_buf.c b/lib/seq_buf.c
index b7477aefff53..165caed5a74e 100644
--- a/lib/seq_buf.c
+++ b/lib/seq_buf.c
@@ -109,9 +109,7 @@ void seq_buf_do_printk(struct seq_buf *s, const char *lvl)
if (s->size == 0 || s->len == 0)
return;
- seq_buf_terminate(s);
-
- start = s->buffer;
+ start = seq_buf_cstr(s);
while ((lf = strchr(start, '\n'))) {
int len = lf - start + 1;
--
2.34.1
[RFC][PATCH] wifi: wil6210: Replace strlcat() usage with seq_buf
The use of strlcat() is fragile at best, and we'd like to remove it from
the available string APIs in the kernel. Instead, use the safer seq_buf
APIs.
Cc: Kalle Valo
Cc: Johannes Berg
Cc: Max Chen
Cc: Yang Shen
Cc: Steven Rostedt
Cc: "Matthew Wilcox (Oracle)"
Cc: Christoph Hellwig
Cc: Justin Stitt
Cc: Kent Overstreet
Cc: Petr Mladek
Cc: Andy Shevchenko
Cc: Rasmus Villemoes
Cc: Sergey Senozhatsky
Cc: Masami Hiramatsu
Cc: Greg Kroah-Hartman
Cc: Arnd Bergmann
Cc: Jonathan Corbet
Cc: Yun Zhou
Cc: Jacob Keller
Cc: Zhen Lei
Cc: linux-trace-ker...@vger.kernel.org
Cc: linux-wirel...@vger.kernel.org
Signed-off-by: Kees Cook
---
This is mainly an example of where/how to use the ongoing seq_buf
refactoring happening in the tracing tree:
https://lore.kernel.org/lkml/20231026170722.work.638-k...@kernel.org/
---
drivers/net/wireless/ath/wil6210/wmi.c | 23 ++-
1 file changed, 10 insertions(+), 13 deletions(-)
diff --git a/drivers/net/wireless/ath/wil6210/wmi.c
b/drivers/net/wireless/ath/wil6210/wmi.c
index 6fdb77d4c59e..45b8c651b8e2 100644
--- a/drivers/net/wireless/ath/wil6210/wmi.c
+++ b/drivers/net/wireless/ath/wil6210/wmi.c
@@ -3159,36 +3159,34 @@ int wmi_suspend(struct wil6210_priv *wil)
return rc;
}
-static void resume_triggers2string(u32 triggers, char *string, int str_size)
+static void resume_triggers2string(u32 triggers, struct seq_buf *s)
{
- string[0] = '\0';
-
if (!triggers) {
- strlcat(string, " UNKNOWN", str_size);
+ seq_buf_puts(s, " UNKNOWN");
return;
}
if (triggers & WMI_RESUME_TRIGGER_HOST)
- strlcat(string, " HOST", str_size);
+ seq_buf_puts(s, " HOST")
if (triggers & WMI_RESUME_TRIGGER_UCAST_RX)
- strlcat(string, " UCAST_RX", str_size);
+ seq_buf_puts(s, " UCAST_RX");
if (triggers & WMI_RESUME_TRIGGER_BCAST_RX)
- strlcat(string, " BCAST_RX", str_size);
+ seq_buf_puts(s, " BCAST_RX");
if (triggers & WMI_RESUME_TRIGGER_WMI_EVT)
- strlcat(string, " WMI_EVT", str_size);
+ seq_buf_puts(s, " WMI_EVT");
if (triggers & WMI_RESUME_TRIGGER_DISCONNECT)
- strlcat(string, " DISCONNECT", str_size);
+ seq_buf_puts(s, " DISCONNECT");
}
int wmi_resume(struct wil6210_priv *wil)
{
struct wil6210_vif *vif = ndev_to_vif(wil->main_ndev);
int rc;
- char string[100];
+ DECLARE_SEQ_BUF(s, 100);
struct {
struct wmi_cmd_hdr wmi;
struct wmi_traffic_resume_event evt;
@@ -3203,10 +3201,9 @@ int wmi_resume(struct wil6210_priv *wil)
WIL_WAIT_FOR_SUSPEND_RESUME_COMP);
if (rc)
return rc;
- resume_triggers2string(le32_to_cpu(reply.evt.resume_triggers), string,
- sizeof(string));
+ resume_triggers2string(le32_to_cpu(reply.evt.resume_triggers), s);
wil_dbg_pm(wil, "device resume %s, resume triggers:%s (0x%x)\n",
- reply.evt.status ? "failed" : "passed", string,
+ reply.evt.status ? "failed" : "passed", seq_buf_cstr(s),
le32_to_cpu(reply.evt.resume_triggers));
return reply.evt.status;
--
2.34.1
Re: [PATCH v2] EDAC/thunderx: Fix some potential buffer overflow in thunderx_ocx_com_threaded_isr()
On Tue, Oct 24, 2023 at 04:39:36PM -0700, Kees Cook wrote: > As the replacements get longer, I would encourage you to use seq_buf > instead -- it does all the length math internally. For example: There's some ongoing work to make seq_buf easier to use: https://lore.kernel.org/lkml/20231026170722.work.638-k...@kernel.org/ Perhaps we can add an "alloc" and "free" pair too, to handle this case: > msg = kmalloc(OCX_MESSAGE_SIZE, GFP_KERNEL); > seq_buf_init(&s, msg, OCX_MESSAGE_SIZE); But perhaps it's overkill... -- Kees Cook
Re: [PATCH v3 1/2] wifi: brcm80211: replace deprecated strncpy with strscpy
On Tue, Oct 17, 2023 at 08:11:28PM +, Justin Stitt wrote:
> Let's move away from using strncpy and instead favor a less ambiguous
> and more robust interface.
>
> For ifp->ndev->name, we expect ifp->ndev->name to be NUL-terminated based
> on its use in format strings within core.c:
> 67 | char *brcmf_ifname(struct brcmf_if *ifp)
> 68 | {
> 69 |if (!ifp)
> 70 |return "";
> 71 |
> 72 |if (ifp->ndev)
> 73 |return ifp->ndev->name;
> 74 |
> 75 |return "";
> 76 | }
> ...
> 288 | static netdev_tx_t brcmf_netdev_start_xmit(struct sk_buff *skb,
> 289 | struct net_device *ndev) {
> ...
> 330 | brcmf_dbg(INFO, "%s: insufficient headroom (%d)\n",
> 331 | brcmf_ifname(ifp), head_delta);
> ...
> 336 | bphy_err(drvr, "%s: failed to expand headroom\n",
> 337 |brcmf_ifname(ifp));
>
> For di->name, we expect di->name to be NUL-terminated based on its usage
> with format strings:
> | brcms_dbg_dma(di->core,
> | "%s: DMA64 tx doesn't have AE set\n",
> | di->name);
>
> Looking at its allocation we can see that it is already zero-allocated
> which means NUL-padding is not required:
> | di = kzalloc(sizeof(struct dma_info), GFP_ATOMIC);
>
> For wlc->modulecb[i].name, we expect each name in wlc->modulecb to be
> NUL-terminated based on their usage with strcmp():
> | if (!strcmp(wlc->modulecb[i].name, name) &&
>
> NUL-padding is not required as wlc is zero-allocated in:
> brcms_c_attach_malloc() ->
> | wlc = kzalloc(sizeof(struct brcms_c_info), GFP_ATOMIC);
>
> For all these cases, a suitable replacement is `strscpy` due to the fact
> that it guarantees NUL-termination on the destination buffer without
> unnecessarily NUL-padding.
>
> Signed-off-by: Justin Stitt
Good; this looks like standard direct replacements.
Reviewed-by: Kees Cook
--
Kees Cook
Re: [PATCH v3 2/2] wifi: brcmsmac: replace deprecated strncpy with memcpy
On Wed, Oct 18, 2023 at 05:03:02PM -0700, Kees Cook wrote: > On Tue, Oct 17, 2023 at 08:11:29PM +, Justin Stitt wrote: > > Let's move away from using strncpy and instead use the more obvious > > interface for this context. > [...] > So, this change results in the same behavior ... I should have included my r-b tag: Reviewed-by: Kees Cook -- Kees Cook
Re: [PATCH] airo: replace deprecated strncpy with strscpy_pad
On Tue, Oct 17, 2023 at 03:51:58PM -0700, Jeff Johnson wrote:
> On 10/17/2023 2:12 PM, Justin Stitt wrote:
> > strncpy() is deprecated for use on NUL-terminated destination strings
> > [1] and as such we should prefer more robust and less ambiguous string
> > interfaces.
> >
> > `extra` is clearly supposed to be NUL-terminated which is evident by the
> > manual NUL-byte assignment as well as its immediate usage with strlen().
> >
> > Moreover, let's NUL-pad since there is deliberate effort (48 instances)
> > made elsewhere to zero-out buffers in these getters and setters:
> > 6050 | memset(local->config.nodeName, 0, sizeof(local->config.nodeName));
> > 6130 | memset(local->config.rates, 0, 8);
> > 6139 | memset(local->config.rates, 0, 8);
> > 6414 | memset(key.key, 0, MAX_KEY_SIZE);
> > 6497 | memset(extra, 0, 16);
> > (to be clear, strncpy also NUL-padded -- we are matching that behavior)
> >
> > Considering the above, a suitable replacement is `strscpy_pad` due to
> > the fact that it guarantees both NUL-termination and NUL-padding on the
> > destination buffer.
> >
> > Technically, we can now copy one less byte into `extra` as we cannot
> > determine the sizeof `extra` at compile time and the hard-coded value of
> > 16 means that strscpy_pad() will automatically truncate and set the byte
> > at offset 15 to NUL. However, the current code manually sets a
> > NUL-byte at offset 16. If this is an issue, the solution is to change
> > the hard-coded magic number to 17 instead of 16. I didn't do this in
> > this patch because a hard-coded 17 seems bad (even more so than 16).
>
> this function is a wext handler. In wext-core.c we have:
> static const struct iw_ioctl_description standard_ioctl[] = {
> ...
> [IW_IOCTL_IDX(SIOCGIWNICKN)] = {
> .header_type= IW_HEADER_TYPE_POINT,
> .token_size = 1,
> .max_tokens = IW_ESSID_MAX_SIZE,
> },
>
> So the buffer size is (strangely) IW_ESSID_MAX_SIZE if you want to use that
> for the buffer size
Yeah, that seems like a good refactor to do at the same time.
> > - strncpy(extra, local->config.nodeName, 16);
> > - extra[16] = '\0';
> > + strscpy_pad(extra, local->config.nodeName, 16);
Justin, can you respin this with the open-coded "16" updated?
-Kees
--
Kees Cook
Re: [PATCH] seq_buf: Introduce DECLARE_SEQ_BUF and seq_buf_cstr()
On Thu, 26 Oct 2023 10:07:28 -0700 Kees Cook wrote: > Solve two ergonomic issues with struct seq_buf: "ergonomic"? Does it cause carpal tunnel? ;-) > > 1) Too much boilerplate is required to initialize: > > struct seq_buf s; > char buf[32]; > > seq_buf_init(s, buf, sizeof(buf)); > > Instead, we can build this directly on the stack. Provide > DECLARE_SEQ_BUF() macro to do this: > > DECLARE_SEQ_BUF(s, 32); > > 2) %NUL termination is fragile and requires 2 steps to get a valid >C String (and is a layering violation exposing the "internals" of >seq_buf): > > seq_buf_terminate(s); > do_something(s->buffer); > > Instead, we can just return s->buffer direction after terminating it > in refactored seq_buf_terminate(), now known as seq_buf_cstr(): > > do_soemthing(seq_buf_cstr(s)); Do we really need to call it _cstr? Why not just have seq_buf_str() ? I mean, this is C, do we need to state that in the name too? BTW, I'm perfectly fine with this change, just the naming I have issues with. -- Steve
Re: [PATCH] scsi: hpsa: replace deprecated strncpy with strscpy/kmemdup_nul
On Thu, Oct 26, 2023 at 01:47:32AM +, Justin Stitt wrote:
> strncpy() is deprecated for use on NUL-terminated destination strings
> [1] and as such we should prefer more robust and less ambiguous string
> interfaces.
>
> This whole process of 1) determining smaller length so we don't overread
> the buffer and 2) manually NUL-terminating our buffer so we can use in
> string APIs is handled implicitly by strscpy().
>
> Therefore, a suitable replacement is `strscpy` [2] due to the fact that
> it guarantees NUL-termination on the destination buffer without
> unnecessarily NUL-padding.
>
> For the last two strncpy() use cases in init_driver_version(), we can
> actually drop this function entirely.
>
> Firstly, we are kmalloc()'ing driver_version. Then, we are calling
> init_driver_version() which memset's it to 0 followed by a strncpy().
> This pattern of 1) allocating memory for a string, 2) setting all bytes
> to NUL, 3) copy bytes from another string + ensure NUL-padded
> destination is just an open-coded kmemdup_nul().
>
> The last case involves swapping kmalloc_array() for kcalloc() to give us
> a zero-filled two-element array for both old_driver_version and
> driver_version without needing the memset from init_driver_version().
>
> Now this code is easier to read and less fragile (no more ... - 1's) or
> min length checks and now we have guaranteed NUL-termination everywhere!
>
> Although perhaps there should be a macro for:
>
> Link:
> https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings
> [1]
> Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html
> [2]
> Link: https://github.com/KSPP/linux/issues/90
> Cc: linux-hardening@vger.kernel.org
> Cc: Kees Cook
> Signed-off-by: Justin Stitt
> ---
> Note: build-tested only.
>
> Found with: $ rg "strncpy\("
> ---
> drivers/scsi/hpsa.c | 29 +++--
> 1 file changed, 11 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
> index af18d20f3079..3376d4614fe5 100644
> --- a/drivers/scsi/hpsa.c
> +++ b/drivers/scsi/hpsa.c
> @@ -452,16 +452,15 @@ static ssize_t
> host_store_hp_ssd_smart_path_status(struct device *dev,
>struct device_attribute *attr,
>const char *buf, size_t count)
> {
> - int status, len;
> + int status;
> struct ctlr_info *h;
> struct Scsi_Host *shost = class_to_shost(dev);
> char tmpbuf[10];
>
> if (!capable(CAP_SYS_ADMIN) || !capable(CAP_SYS_RAWIO))
> return -EACCES;
> - len = count > sizeof(tmpbuf) - 1 ? sizeof(tmpbuf) - 1 : count;
> - strncpy(tmpbuf, buf, len);
> - tmpbuf[len] = '\0';
> + strscpy(tmpbuf, buf, count);
This is wrong -- "count" isn't the size of tmpbuf -- it's the size of
the source, i.e. strlen(buf).
> +
> if (sscanf(tmpbuf, "%d", &status) != 1)
> return -EINVAL;
And this is immediately using the tmpbuf for getting an int. All of this
should be replaced by kstrtoint().
> h = shost_to_hba(shost);
> @@ -476,16 +475,15 @@ static ssize_t host_store_raid_offload_debug(struct
> device *dev,
>struct device_attribute *attr,
>const char *buf, size_t count)
> {
> - int debug_level, len;
> + int debug_level;
> struct ctlr_info *h;
> struct Scsi_Host *shost = class_to_shost(dev);
> char tmpbuf[10];
>
> if (!capable(CAP_SYS_ADMIN) || !capable(CAP_SYS_RAWIO))
> return -EACCES;
> - len = count > sizeof(tmpbuf) - 1 ? sizeof(tmpbuf) - 1 : count;
> - strncpy(tmpbuf, buf, len);
> - tmpbuf[len] = '\0';
> + strscpy(tmpbuf, buf, count);
> +
> if (sscanf(tmpbuf, "%d", &debug_level) != 1)
> return -EINVAL;
Same thing here.
> if (debug_level < 0)
> @@ -7234,24 +7232,19 @@ static int hpsa_controller_hard_reset(struct pci_dev
> *pdev,
> return 0;
> }
>
> -static void init_driver_version(char *driver_version, int len)
> -{
> - memset(driver_version, 0, len);
> - strncpy(driver_version, HPSA " " HPSA_DRIVER_VERSION, len - 1);
> -}
> -
> static int write_driver_ver_to_cfgtable(struct CfgTable __iomem *cfgtable)
> {
> char *driver_version;
> int i, size = sizeof(cfgtable->driver_version);
>
> - driver_version = kmalloc(size, GFP_KERNEL);
> + driver_version = kmemdup_nul(HPSA " " HPSA_DRIVER_VERSION, size,
> + GFP_KERNEL);
"size" isn't the length of the string here, so this results in an
over-read from the .data segment:
drivers/scsi/hpsa.c:#define HPSA "hpsa"
drivers/scsi/hpsa.c:#define HPSA_DRIVER_VERSION "3.4.20-200"
strlen(HSPA " " HPSA_DRIVER_VERSION) == 15 (16 with %NUL terminator)
sizeof(cfgtable->driver_version) == 32:
struct CfgTable {
...
u8 driver_version[32]
Re: [PATCH v2] scsi: elx: libefc: replace deprecated strncpy with strscpy_pad/memcpy
On Thu, Oct 26, 2023 at 01:53:13AM +, Justin Stitt wrote: > strncpy() is deprecated for use on NUL-terminated destination strings > [1] and as such we should prefer more robust and less ambiguous string > interfaces. > > To keep node->current_state_name and node->prev_state_name NUL-padded > and NUL-terminated let's use strscpy_pad() as this implicitly provides > both. > > For the swap between the two, a simple memcpy will suffice. > > Link: > https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings > [1] > Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html > [2] > Link: https://github.com/KSPP/linux/issues/90 > Cc: linux-hardening@vger.kernel.org > Signed-off-by: Justin Stitt Thanks! I think this looks good now. Reviewed-by: Kees Cook -- Kees Cook
Re: [PATCH] seq_buf: Introduce DECLARE_SEQ_BUF and seq_buf_cstr()
On Thu, Oct 26, 2023 at 01:38:50PM -0400, Steven Rostedt wrote: > On Thu, 26 Oct 2023 10:07:28 -0700 > Kees Cook wrote: > > > Solve two ergonomic issues with struct seq_buf: > > "ergonomic"? Does it cause carpal tunnel? ;-) > > > > > 1) Too much boilerplate is required to initialize: > > > > struct seq_buf s; > > char buf[32]; > > > > seq_buf_init(s, buf, sizeof(buf)); > > > > Instead, we can build this directly on the stack. Provide > > DECLARE_SEQ_BUF() macro to do this: > > > > DECLARE_SEQ_BUF(s, 32); > > > > 2) %NUL termination is fragile and requires 2 steps to get a valid > >C String (and is a layering violation exposing the "internals" of > >seq_buf): > > > > seq_buf_terminate(s); > > do_something(s->buffer); > > > > Instead, we can just return s->buffer direction after terminating it > > in refactored seq_buf_terminate(), now known as seq_buf_cstr(): > > > > do_soemthing(seq_buf_cstr(s)); > > Do we really need to call it _cstr? Why not just have seq_buf_str() ? > > I mean, this is C, do we need to state that in the name too? I'm fine either way. I did that just to make the distinction between our length-managed string of characters interface (seq_buf), and the %NUL-terminated string of characters (traditionally called "C String" in other languages). And it was still shorter than "seq_buf_terminate(s); s->buffer" ;) > BTW, I'm perfectly fine with this change, just the naming I have issues > with. Cool; thanks for looking at it! -- Kees Cook
Re: [PATCH] seq_buf: Introduce DECLARE_SEQ_BUF and seq_buf_cstr()
On Thu, 26 Oct 2023 10:54:26 -0700 Kees Cook wrote: > > Do we really need to call it _cstr? Why not just have seq_buf_str() ? > > > > I mean, this is C, do we need to state that in the name too? > > I'm fine either way. I did that just to make the distinction between our > length-managed string of characters interface (seq_buf), and the > %NUL-terminated string of characters (traditionally called "C String" in > other languages). And it was still shorter than "seq_buf_terminate(s); > s->buffer" ;) Do you believe that people might get confused with it as seq_buf_str()? Can you envision that we would want a seq_buf_str() and seq_buf_cstr() that do something different? -- Steve
Re: [PATCH] seq_buf: Introduce DECLARE_SEQ_BUF and seq_buf_cstr()
On Thu, Oct 26, 2023 at 02:02:47PM -0400, Steven Rostedt wrote: > On Thu, 26 Oct 2023 10:54:26 -0700 > Kees Cook wrote: > > > > Do we really need to call it _cstr? Why not just have seq_buf_str() ? > > > > > > I mean, this is C, do we need to state that in the name too? > > > > I'm fine either way. I did that just to make the distinction between our > > length-managed string of characters interface (seq_buf), and the > > %NUL-terminated string of characters (traditionally called "C String" in > > other languages). And it was still shorter than "seq_buf_terminate(s); > > s->buffer" ;) > > Do you believe that people might get confused with it as seq_buf_str()? > > Can you envision that we would want a seq_buf_str() and seq_buf_cstr() that > do something different? No, I see your point. Like I said, I don't care either way. I was just explaining why I did it that way. "string" means a lot of things to different people. "C String" is unambiguous, and I try to be unambiguous whenever possible. :) I'll send a v2 as seq_buf_str()... -- Kees Cook
Re: [PATCH] seq_buf: Introduce DECLARE_SEQ_BUF and seq_buf_cstr()
On Thu, 26 Oct 2023 12:35:43 -0700 Kees Cook wrote: > I'll send a v2 as seq_buf_str()... Thanks. -- Steve
[PATCH v2] seq_buf: Introduce DECLARE_SEQ_BUF and seq_buf_str()
Solve two ergonomic issues with struct seq_buf;
1) Too much boilerplate is required to initialize:
struct seq_buf s;
char buf[32];
seq_buf_init(s, buf, sizeof(buf));
Instead, we can build this directly on the stack. Provide
DECLARE_SEQ_BUF() macro to do this:
DECLARE_SEQ_BUF(s, 32);
2) %NUL termination is fragile and requires 2 steps to get a valid
C String (and is a layering violation exposing the "internals" of
seq_buf):
seq_buf_terminate(s);
do_something(s->buffer);
Instead, we can just return s->buffer direction after terminating it
in refactored seq_buf_terminate(), now known as seq_buf_str():
do_soemthing(seq_buf_str(s));
Cc: Steven Rostedt
Cc: "Matthew Wilcox (Oracle)"
Cc: Christoph Hellwig
Cc: Justin Stitt
Cc: Kent Overstreet
Cc: Petr Mladek
Cc: Andy Shevchenko
Cc: Rasmus Villemoes
Cc: Sergey Senozhatsky
Cc: Masami Hiramatsu
Cc: Greg Kroah-Hartman
Cc: Arnd Bergmann
Cc: Jonathan Corbet
Cc: Yun Zhou
Cc: Jacob Keller
Cc: Zhen Lei
Cc: linux-trace-ker...@vger.kernel.org
Signed-off-by: Kees Cook
---
v2 - rename seq_buf_cstr() to seq_buf_str() (rostedt)
v1 - https://lore.kernel.org/all/20231026170722.work.638-k...@kernel.org/
---
include/linux/seq_buf.h | 19 +++
kernel/trace/trace.c| 11 +--
lib/seq_buf.c | 4 +---
3 files changed, 17 insertions(+), 17 deletions(-)
diff --git a/include/linux/seq_buf.h b/include/linux/seq_buf.h
index 8483e4b2d0d2..71eb4d624308 100644
--- a/include/linux/seq_buf.h
+++ b/include/linux/seq_buf.h
@@ -21,9 +21,16 @@ struct seq_buf {
size_t len;
};
+#define DECLARE_SEQ_BUF(NAME, SIZE)\
+ char __ ## NAME ## _buffer[SIZE] = ""; \
+ struct seq_buf NAME = { .buffer = &__ ## NAME ## _buffer, \
+ .size = SIZE }
+
static inline void seq_buf_clear(struct seq_buf *s)
{
s->len = 0;
+ if (s->size)
+ s->buffer[0] = '\0';
}
static inline void
@@ -69,8 +76,8 @@ static inline unsigned int seq_buf_used(struct seq_buf *s)
}
/**
- * seq_buf_terminate - Make sure buffer is nul terminated
- * @s: the seq_buf descriptor to terminate.
+ * seq_buf_str - get %NUL-terminated C string from seq_buf
+ * @s: the seq_buf handle
*
* This makes sure that the buffer in @s is nul terminated and
* safe to read as a string.
@@ -81,16 +88,20 @@ static inline unsigned int seq_buf_used(struct seq_buf *s)
*
* After this function is called, s->buffer is safe to use
* in string operations.
+ *
+ * Returns @s->buf after making sure it is terminated.
*/
-static inline void seq_buf_terminate(struct seq_buf *s)
+static inline char *seq_buf_str(struct seq_buf *s)
{
if (WARN_ON(s->size == 0))
- return;
+ return "";
if (seq_buf_buffer_left(s))
s->buffer[s->len] = 0;
else
s->buffer[s->size - 1] = 0;
+
+ return s->buffer;
}
/**
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index d629065c2383..2539cfc20a97 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -3828,15 +3828,6 @@ static bool trace_safe_str(struct trace_iterator *iter,
const char *str,
return false;
}
-static const char *show_buffer(struct trace_seq *s)
-{
- struct seq_buf *seq = &s->seq;
-
- seq_buf_terminate(seq);
-
- return seq->buffer;
-}
-
static DEFINE_STATIC_KEY_FALSE(trace_no_verify);
static int test_can_verify_check(const char *fmt, ...)
@@ -3976,7 +3967,7 @@ void trace_check_vprintf(struct trace_iterator *iter,
const char *fmt,
*/
if (WARN_ONCE(!trace_safe_str(iter, str, star, len),
"fmt: '%s' current_buffer: '%s'",
- fmt, show_buffer(&iter->seq))) {
+ fmt, seq_buf_str(&iter->seq.seq))) {
int ret;
/* Try to safely read the string */
diff --git a/lib/seq_buf.c b/lib/seq_buf.c
index b7477aefff53..23518f77ea9c 100644
--- a/lib/seq_buf.c
+++ b/lib/seq_buf.c
@@ -109,9 +109,7 @@ void seq_buf_do_printk(struct seq_buf *s, const char *lvl)
if (s->size == 0 || s->len == 0)
return;
- seq_buf_terminate(s);
-
- start = s->buffer;
+ start = seq_buf_str(s);
while ((lf = strchr(start, '\n'))) {
int len = lf - start + 1;
--
2.34.1
Re: [PATCH v2] seq_buf: Introduce DECLARE_SEQ_BUF and seq_buf_str()
On Thu, 26 Oct 2023 12:40:37 -0700
Kees Cook wrote:
> @@ -81,16 +88,20 @@ static inline unsigned int seq_buf_used(struct seq_buf *s)
> *
> * After this function is called, s->buffer is safe to use
> * in string operations.
> + *
> + * Returns @s->buf after making sure it is terminated.
> */
> -static inline void seq_buf_terminate(struct seq_buf *s)
> +static inline char *seq_buf_str(struct seq_buf *s)
Looking at show_buffer() (below), I wonder if this should be:
static inline const char *seq_buf_str() ?
I mean, it can be modified, but do we want to allow that?
-- Steve
> {
> if (WARN_ON(s->size == 0))
> - return;
> + return "";
>
> if (seq_buf_buffer_left(s))
> s->buffer[s->len] = 0;
> else
> s->buffer[s->size - 1] = 0;
> +
> + return s->buffer;
> }
>
> /**
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index d629065c2383..2539cfc20a97 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -3828,15 +3828,6 @@ static bool trace_safe_str(struct trace_iterator
> *iter, const char *str,
> return false;
> }
>
> -static const char *show_buffer(struct trace_seq *s)
> -{
> - struct seq_buf *seq = &s->seq;
> -
> - seq_buf_terminate(seq);
> -
> - return seq->buffer;
> -}
> -
> static DEFINE_STATIC_KEY_FALSE(trace_no_verify);
>
Re: [PATCH v2] seq_buf: Introduce DECLARE_SEQ_BUF and seq_buf_str()
On Thu, Oct 26, 2023 at 12:40:37PM -0700, Kees Cook wrote:
> Solve two ergonomic issues with struct seq_buf;
>
> 1) Too much boilerplate is required to initialize:
>
> struct seq_buf s;
> char buf[32];
>
> seq_buf_init(s, buf, sizeof(buf));
>
> Instead, we can build this directly on the stack. Provide
> DECLARE_SEQ_BUF() macro to do this:
>
> DECLARE_SEQ_BUF(s, 32);
>
> 2) %NUL termination is fragile and requires 2 steps to get a valid
>C String (and is a layering violation exposing the "internals" of
>seq_buf):
>
> seq_buf_terminate(s);
> do_something(s->buffer);
>
> Instead, we can just return s->buffer direction after terminating it
> in refactored seq_buf_terminate(), now known as seq_buf_str():
>
> do_soemthing(seq_buf_str(s));
...
> +#define DECLARE_SEQ_BUF(NAME, SIZE) \
> + char __ ## NAME ## _buffer[SIZE] = ""; \
> + struct seq_buf NAME = { .buffer = &__ ## NAME ## _buffer, \
> + .size = SIZE }
Hmm... Wouldn't be more readable to have it as
#define DECLARE_SEQ_BUF(NAME, SIZE) \
char __ ## NAME ## _buffer[SIZE] = ""; \
struct seq_buf NAME = { \
.buffer = &__ ## NAME ## _buffer, \
.size = SIZE, \
}
?
...
> +static inline char *seq_buf_str(struct seq_buf *s)
> {
> if (WARN_ON(s->size == 0))
> - return;
> + return "";
I'm wondering why it's a problem to have an empty string?
> if (seq_buf_buffer_left(s))
> s->buffer[s->len] = 0;
> else
> s->buffer[s->size - 1] = 0;
> +
> + return s->buffer;
> }
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v2] seq_buf: Introduce DECLARE_SEQ_BUF and seq_buf_str()
On Thu, 26 Oct 2023 23:20:15 +0300
Andy Shevchenko wrote:
> > +#define DECLARE_SEQ_BUF(NAME, SIZE)
> > \
> > + char __ ## NAME ## _buffer[SIZE] = ""; \
> > + struct seq_buf NAME = { .buffer = &__ ## NAME ## _buffer, \
> > + .size = SIZE }
>
> Hmm... Wouldn't be more readable to have it as
>
> #define DECLARE_SEQ_BUF(NAME, SIZE) \
> char __ ## NAME ## _buffer[SIZE] = ""; \
> struct seq_buf NAME = { \
> .buffer = &__ ## NAME ## _buffer, \
> .size = SIZE, \
> }
>
> ?
I agree with the above.
>
> ...
>
> > +static inline char *seq_buf_str(struct seq_buf *s)
> > {
> > if (WARN_ON(s->size == 0))
> > - return;
> > + return "";
>
> I'm wondering why it's a problem to have an empty string?
Not sure what you mean? With s->size = 0, s->buffer may not have been
assigned. That shouldn't be the case, but it does make it more robust.
-- Steve
>
> > if (seq_buf_buffer_left(s))
> > s->buffer[s->len] = 0;
> > else
> > s->buffer[s->size - 1] = 0;
> > +
> > + return s->buffer;
> > }
>
Re: [RFC][PATCH] wifi: wil6210: Replace strlcat() usage with seq_buf
On Thu, Oct 26, 2023 at 10:13 AM Kees Cook wrote:
>
> The use of strlcat() is fragile at best, and we'd like to remove it from
> the available string APIs in the kernel. Instead, use the safer seq_buf
> APIs.
>
> Cc: Kalle Valo
> Cc: Johannes Berg
> Cc: Max Chen
> Cc: Yang Shen
> Cc: Steven Rostedt
> Cc: "Matthew Wilcox (Oracle)"
> Cc: Christoph Hellwig
> Cc: Justin Stitt
> Cc: Kent Overstreet
> Cc: Petr Mladek
> Cc: Andy Shevchenko
> Cc: Rasmus Villemoes
> Cc: Sergey Senozhatsky
> Cc: Masami Hiramatsu
> Cc: Greg Kroah-Hartman
> Cc: Arnd Bergmann
> Cc: Jonathan Corbet
> Cc: Yun Zhou
> Cc: Jacob Keller
> Cc: Zhen Lei
> Cc: linux-trace-ker...@vger.kernel.org
> Cc: linux-wirel...@vger.kernel.org
> Signed-off-by: Kees Cook
> ---
> This is mainly an example of where/how to use the ongoing seq_buf
> refactoring happening in the tracing tree:
> https://lore.kernel.org/lkml/20231026170722.work.638-k...@kernel.org/
I like it. C-strings and many of their associated apis are dodgy. This
looks like a worthwhile replacement.
I think many of my strncpy -> strscpy replacements could've easily
been something along these lines as well.
Happy to see robustness increasing in the kernel by means
of replacing sketchy C-string stuff.
> ---
> drivers/net/wireless/ath/wil6210/wmi.c | 23 ++-
> 1 file changed, 10 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/wil6210/wmi.c
> b/drivers/net/wireless/ath/wil6210/wmi.c
> index 6fdb77d4c59e..45b8c651b8e2 100644
> --- a/drivers/net/wireless/ath/wil6210/wmi.c
> +++ b/drivers/net/wireless/ath/wil6210/wmi.c
> @@ -3159,36 +3159,34 @@ int wmi_suspend(struct wil6210_priv *wil)
> return rc;
> }
>
> -static void resume_triggers2string(u32 triggers, char *string, int str_size)
> +static void resume_triggers2string(u32 triggers, struct seq_buf *s)
> {
> - string[0] = '\0';
> -
> if (!triggers) {
> - strlcat(string, " UNKNOWN", str_size);
> + seq_buf_puts(s, " UNKNOWN");
> return;
> }
>
> if (triggers & WMI_RESUME_TRIGGER_HOST)
> - strlcat(string, " HOST", str_size);
> + seq_buf_puts(s, " HOST")
>
> if (triggers & WMI_RESUME_TRIGGER_UCAST_RX)
> - strlcat(string, " UCAST_RX", str_size);
> + seq_buf_puts(s, " UCAST_RX");
>
> if (triggers & WMI_RESUME_TRIGGER_BCAST_RX)
> - strlcat(string, " BCAST_RX", str_size);
> + seq_buf_puts(s, " BCAST_RX");
>
> if (triggers & WMI_RESUME_TRIGGER_WMI_EVT)
> - strlcat(string, " WMI_EVT", str_size);
> + seq_buf_puts(s, " WMI_EVT");
>
> if (triggers & WMI_RESUME_TRIGGER_DISCONNECT)
> - strlcat(string, " DISCONNECT", str_size);
> + seq_buf_puts(s, " DISCONNECT");
> }
>
> int wmi_resume(struct wil6210_priv *wil)
> {
> struct wil6210_vif *vif = ndev_to_vif(wil->main_ndev);
> int rc;
> - char string[100];
> + DECLARE_SEQ_BUF(s, 100);
> struct {
> struct wmi_cmd_hdr wmi;
> struct wmi_traffic_resume_event evt;
> @@ -3203,10 +3201,9 @@ int wmi_resume(struct wil6210_priv *wil)
> WIL_WAIT_FOR_SUSPEND_RESUME_COMP);
> if (rc)
> return rc;
> - resume_triggers2string(le32_to_cpu(reply.evt.resume_triggers), string,
> - sizeof(string));
> + resume_triggers2string(le32_to_cpu(reply.evt.resume_triggers), s);
> wil_dbg_pm(wil, "device resume %s, resume triggers:%s (0x%x)\n",
> - reply.evt.status ? "failed" : "passed", string,
> + reply.evt.status ? "failed" : "passed", seq_buf_cstr(s),
>le32_to_cpu(reply.evt.resume_triggers));
>
> return reply.evt.status;
> --
> 2.34.1
>
Thanks
Justin
[PATCH v2] scsi: hpsa: replace deprecated strncpy
strncpy() is deprecated for use on NUL-terminated destination strings
[1] and as such we should prefer more robust and less ambiguous string
interfaces.
Instances of strncpy()'ing a string into a buffer and manually
NUL-terminating followed by sccanf with just "%d" as the format
specifier can be accomplished by strscpy() and kstrtoint().
strscpy() guarantees NUL-termination on the destination buffer and
kstrtoint is better way of getting strings turned into ints.
For the last two strncpy() use cases in init_driver_version(), we can
actually drop this function entirely.
Firstly, we are kmalloc()'ing driver_version. Then, we are calling
init_driver_version() which memset's it to 0 followed by a strncpy().
The pattern is 1) allocating memory for a string, 2) setting all bytes
to NUL, 3) copy bytes from another string + ensure NUL-padded.
For these, we can just stack allocate driver_version and
old_driver_version. This simplifies the code greatly as we don't have
any malloc/free or strncpy's.
Link:
https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings
[1]
Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [2]
Link: https://github.com/KSPP/linux/issues/90
Cc: linux-hardening@vger.kernel.org
Cc: Kees Cook
Signed-off-by: Justin Stitt
---
Changes in v2:
- use stack for buffers (thanks Kees)
- use kstrtoint (thanks Kees)
- Link to v1:
https://lore.kernel.org/r/20231026-strncpy-drivers-scsi-hpsa-c-v1-1-75519d7a1...@google.com
---
Note: build-tested only.
Found with: $ rg "strncpy\("
---
drivers/scsi/hpsa.c | 53 -
1 file changed, 20 insertions(+), 33 deletions(-)
diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
index af18d20f3079..4d42fbb071cf 100644
--- a/drivers/scsi/hpsa.c
+++ b/drivers/scsi/hpsa.c
@@ -452,18 +452,18 @@ static ssize_t host_store_hp_ssd_smart_path_status(struct
device *dev,
struct device_attribute *attr,
const char *buf, size_t count)
{
- int status, len;
+ int status;
struct ctlr_info *h;
struct Scsi_Host *shost = class_to_shost(dev);
char tmpbuf[10];
if (!capable(CAP_SYS_ADMIN) || !capable(CAP_SYS_RAWIO))
return -EACCES;
- len = count > sizeof(tmpbuf) - 1 ? sizeof(tmpbuf) - 1 : count;
- strncpy(tmpbuf, buf, len);
- tmpbuf[len] = '\0';
- if (sscanf(tmpbuf, "%d", &status) != 1)
+
+ strscpy(tmpbuf, buf, sizeof(tmpbuf));
+ if (kstrtoint(tmpbuf, 0, &status))
return -EINVAL;
+
h = shost_to_hba(shost);
h->acciopath_status = !!status;
dev_warn(&h->pdev->dev,
@@ -476,18 +476,18 @@ static ssize_t host_store_raid_offload_debug(struct
device *dev,
struct device_attribute *attr,
const char *buf, size_t count)
{
- int debug_level, len;
+ int debug_level;
struct ctlr_info *h;
struct Scsi_Host *shost = class_to_shost(dev);
char tmpbuf[10];
if (!capable(CAP_SYS_ADMIN) || !capable(CAP_SYS_RAWIO))
return -EACCES;
- len = count > sizeof(tmpbuf) - 1 ? sizeof(tmpbuf) - 1 : count;
- strncpy(tmpbuf, buf, len);
- tmpbuf[len] = '\0';
- if (sscanf(tmpbuf, "%d", &debug_level) != 1)
+
+ strscpy(tmpbuf, buf, sizeof(tmpbuf));
+ if (kstrtoint(tmpbuf, 0, &debug_level))
return -EINVAL;
+
if (debug_level < 0)
debug_level = 0;
h = shost_to_hba(shost);
@@ -7234,25 +7234,15 @@ static int hpsa_controller_hard_reset(struct pci_dev
*pdev,
return 0;
}
-static void init_driver_version(char *driver_version, int len)
-{
- memset(driver_version, 0, len);
- strncpy(driver_version, HPSA " " HPSA_DRIVER_VERSION, len - 1);
-}
-
static int write_driver_ver_to_cfgtable(struct CfgTable __iomem *cfgtable)
{
- char *driver_version;
int i, size = sizeof(cfgtable->driver_version);
+ char driver_version[sizeof(cfgtable->driver_version)] =
+ HPSA " " HPSA_DRIVER_VERSION;
- driver_version = kmalloc(size, GFP_KERNEL);
- if (!driver_version)
- return -ENOMEM;
-
- init_driver_version(driver_version, size);
for (i = 0; i < size; i++)
writeb(driver_version[i], &cfgtable->driver_version[i]);
- kfree(driver_version);
+
return 0;
}
@@ -7268,21 +7258,18 @@ static void read_driver_ver_from_cfgtable(struct
CfgTable __iomem *cfgtable,
static int controller_reset_failed(struct CfgTable __iomem *cfgtable)
{
- char *driver_ver, *old_
[PATCH v2] airo: replace deprecated strncpy with strscpy_pad
strncpy() is deprecated for use on NUL-terminated destination strings
[1] and as such we should prefer more robust and less ambiguous string
interfaces.
`extra` is clearly supposed to be NUL-terminated which is evident by the
manual NUL-byte assignment as well as its immediate usage with strlen().
Moreover, let's NUL-pad since there is deliberate effort (48 instances)
made elsewhere to zero-out buffers in these getters and setters:
6050 | memset(local->config.nodeName, 0, sizeof(local->config.nodeName));
6130 | memset(local->config.rates, 0, 8);
6139 | memset(local->config.rates, 0, 8);
6414 | memset(key.key, 0, MAX_KEY_SIZE);
6497 | memset(extra, 0, 16);
(to be clear, strncpy also NUL-padded -- we are matching that behavior)
Considering the above, a suitable replacement is `strscpy_pad` due to
the fact that it guarantees both NUL-termination and NUL-padding on the
destination buffer.
We can also replace the hard-coded size of "16" to IW_ESSID_MAX_SIZE
because this function is a wext handler.
In wext-core.c we have:
static const struct iw_ioctl_description standard_ioctl[] = {
...
[IW_IOCTL_IDX(SIOCGIWNICKN)] = {
.header_type= IW_HEADER_TYPE_POINT,
.token_size = 1,
.max_tokens = IW_ESSID_MAX_SIZE,
},
So the buffer size is (strangely) IW_ESSID_MAX_SIZE
Link:
https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings
[1]
Link: https://github.com/KSPP/linux/issues/90
Cc: linux-hardening@vger.kernel.org
Signed-off-by: Justin Stitt
---
Changes in v2:
- use IW_ESSID_MAX_SIZE (thanks Jeff, Kees)
- Link to v1:
https://lore.kernel.org/r/20231017-strncpy-drivers-net-wireless-cisco-airo-c-v1-1-e34d5b3b7...@google.com
---
Note: build-tested only.
Found with: $ rg "strncpy\("
---
drivers/net/wireless/cisco/airo.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/net/wireless/cisco/airo.c
b/drivers/net/wireless/cisco/airo.c
index dbd13f7aa3e6..6a099642e854 100644
--- a/drivers/net/wireless/cisco/airo.c
+++ b/drivers/net/wireless/cisco/airo.c
@@ -6067,8 +6067,7 @@ static int airo_get_nick(struct net_device *dev,
struct airo_info *local = dev->ml_priv;
readConfigRid(local, 1);
- strncpy(extra, local->config.nodeName, 16);
- extra[16] = '\0';
+ strscpy_pad(extra, local->config.nodeName, IW_ESSID_MAX_SIZE);
dwrq->length = strlen(extra);
return 0;
---
base-commit: 58720809f52779dc0f08e53e54b014209d13eebb
change-id: 20231017-strncpy-drivers-net-wireless-cisco-airo-c-d09cd0500a6e
Best regards,
--
Justin Stitt
[PATCH v2] iio: sx9324: avoid copying property strings
We're doing some needless string copies when trying to assign the proper `prop` string. We can make `prop` a const char* and simply assign to string literals. For the case where a format string is used, let's allocate some memory via kasprintf() and point prop to it. This also cleans up some deprecated strncpy() uses [1]. Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1] Link: https://github.com/KSPP/linux/issues/90 Cc: linux-hardening@vger.kernel.org Signed-off-by: Justin Stitt --- Changes in v2: - make prop a const char* and do simple assignments (thanks Jonathan) - rebase onto 3a568e3a961ba330 - Link to v1: https://lore.kernel.org/r/20230921-strncpy-drivers-iio-proximity-sx9324-c-v1-1-4e8d28fd1...@google.com --- Note: build-tested --- drivers/iio/proximity/sx9324.c | 17 +++-- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/drivers/iio/proximity/sx9324.c b/drivers/iio/proximity/sx9324.c index 438f9c9aba6e..c8547035cb47 100644 --- a/drivers/iio/proximity/sx9324.c +++ b/drivers/iio/proximity/sx9324.c @@ -885,7 +885,7 @@ sx9324_get_default_reg(struct device *dev, int idx, #define SX9324_RESOLUTION_DEF "semtech,ph01-resolution" #define SX9324_PROXRAW_DEF "semtech,ph01-proxraw-strength" unsigned int pin_defs[SX9324_NUM_PINS]; - char prop[] = SX9324_PROXRAW_DEF; + const char *prop = SX9324_PROXRAW_DEF; u32 start = 0, raw = 0, pos = 0; int ret, count, ph, pin; const char *res; @@ -899,7 +899,7 @@ sx9324_get_default_reg(struct device *dev, int idx, case SX9324_REG_AFE_PH2: case SX9324_REG_AFE_PH3: ph = reg_def->reg - SX9324_REG_AFE_PH0; - snprintf(prop, ARRAY_SIZE(prop), "semtech,ph%d-pin", ph); + prop = kasprintf(GFP_KERNEL, "semtech,ph%d-pin", ph); count = device_property_count_u32(dev, prop); if (count != ARRAY_SIZE(pin_defs)) @@ -913,6 +913,7 @@ sx9324_get_default_reg(struct device *dev, int idx, raw |= (pin_defs[pin] << (2 * pin)) & SX9324_REG_AFE_PH0_PIN_MASK(pin); reg_def->def = raw; + kfree(prop); break; case SX9324_REG_AFE_CTRL0: ret = device_property_read_string(dev, @@ -937,11 +938,9 @@ sx9324_get_default_reg(struct device *dev, int idx, case SX9324_REG_AFE_CTRL4: case SX9324_REG_AFE_CTRL7: if (reg_def->reg == SX9324_REG_AFE_CTRL4) - strncpy(prop, "semtech,ph01-resolution", - ARRAY_SIZE(prop)); + prop = "semtech,ph01-resolution"; else - strncpy(prop, "semtech,ph23-resolution", - ARRAY_SIZE(prop)); + prop = "semtech,ph23-resolution"; ret = device_property_read_u32(dev, prop, &raw); if (ret) @@ -1012,11 +1011,9 @@ sx9324_get_default_reg(struct device *dev, int idx, case SX9324_REG_PROX_CTRL0: case SX9324_REG_PROX_CTRL1: if (reg_def->reg == SX9324_REG_PROX_CTRL0) - strncpy(prop, "semtech,ph01-proxraw-strength", - ARRAY_SIZE(prop)); + prop = "semtech,ph01-proxraw-strength"; else - strncpy(prop, "semtech,ph23-proxraw-strength", - ARRAY_SIZE(prop)); + prop = "semtech,ph23-proxraw-strength"; ret = device_property_read_u32(dev, prop, &raw); if (ret) break; --- base-commit: 3a568e3a961ba330091cd031647e4c303fa0badb change-id: 20230921-strncpy-drivers-iio-proximity-sx9324-c-8c3437676039 Best regards, -- Justin Stitt
Re: [PATCH] iio: sx9324: replace deprecated strncpy
Hi Jonathan, On Sat, Sep 23, 2023 at 10:47 AM Jonathan Cameron wrote: > > On Thu, 21 Sep 2023 07:01:01 + > Justin Stitt wrote: > > > `strncpy` is deprecated for use on NUL-terminated destination strings [1]. > > > > We should prefer more robust and less ambiguous string interfaces. > > > > `prop` is defined as this string literal with size 30 (including null): > > | #define SX9324_PROXRAW_DEF "semtech,ph01-proxraw-strength" > > | char prop[] = SX9324_PROXRAW_DEF; > > > > Each of the strncpy->strscpy replacements involve string literals with a > > size less than 30 which means there are no current problems with how > > strncpy is used. However, let's move away from using strncpy entirely. > > > > A suitable replacement is `strscpy` [2] due to the fact that it > > guarantees NUL-termination on the destination buffer without > > unnecessarily NUL-padding. > > > > Moreover, let's opt for the more conventional `sizeof()` as opposed to > > `ARRAY_SIZE` for these simple strings. > > > > Link: > > https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings > > [1] > > Link: > > https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [2] > > Link: https://github.com/KSPP/linux/issues/90 > > Cc: linux-hardening@vger.kernel.org > > Signed-off-by: Justin Stitt > > --- > > FWIW: It seems fragile to base future `prop` stores on the > > size of it's default string. > > Agreed - can we just get rid of the copying? Just set a const char * > to point to appropriate string instead and use that? > > The dance is just about reasonable for the case where there is > a string format being used to fill in the numbers, here I can't see any > advantage at all. (for the other case, a kasprintf() or similar is probably > more appropriate anyway) given this isn't a particular hot path. I sent a [v2]! Can you see if this matches your vision here? > > Jonathan > > > > Note: build-tested > > --- > > drivers/iio/proximity/sx9324.c | 12 > > 1 file changed, 4 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/iio/proximity/sx9324.c b/drivers/iio/proximity/sx9324.c > > index 438f9c9aba6e..25ac2733bcef 100644 > > --- a/drivers/iio/proximity/sx9324.c > > +++ b/drivers/iio/proximity/sx9324.c > > @@ -937,11 +937,9 @@ sx9324_get_default_reg(struct device *dev, int idx, > > case SX9324_REG_AFE_CTRL4: > > case SX9324_REG_AFE_CTRL7: > > if (reg_def->reg == SX9324_REG_AFE_CTRL4) > > - strncpy(prop, "semtech,ph01-resolution", > > - ARRAY_SIZE(prop)); > > + strscpy(prop, "semtech,ph01-resolution", > > sizeof(prop)); > > else > > - strncpy(prop, "semtech,ph23-resolution", > > - ARRAY_SIZE(prop)); > > + strscpy(prop, "semtech,ph23-resolution", > > sizeof(prop)); > > > > ret = device_property_read_u32(dev, prop, &raw); > > if (ret) > > @@ -1012,11 +1010,9 @@ sx9324_get_default_reg(struct device *dev, int idx, > > case SX9324_REG_PROX_CTRL0: > > case SX9324_REG_PROX_CTRL1: > > if (reg_def->reg == SX9324_REG_PROX_CTRL0) > > - strncpy(prop, "semtech,ph01-proxraw-strength", > > - ARRAY_SIZE(prop)); > > + strscpy(prop, "semtech,ph01-proxraw-strength", > > sizeof(prop)); > > else > > - strncpy(prop, "semtech,ph23-proxraw-strength", > > - ARRAY_SIZE(prop)); > > + strscpy(prop, "semtech,ph23-proxraw-strength", > > sizeof(prop)); > > ret = device_property_read_u32(dev, prop, &raw); > > if (ret) > > break; > > > > --- > > base-commit: 2cf0f715623872823a72e451243bbf555d10d032 > > change-id: 20230921-strncpy-drivers-iio-proximity-sx9324-c-8c3437676039 > > > > Best regards, > > -- > > Justin Stitt > > > [v2]: https://lore.kernel.org/r/20231026-strncpy-drivers-iio-proximity-sx9324-c-v2-1-cee6e5db7...@google.com Thanks Justin
Re: [PATCH v2] airo: replace deprecated strncpy with strscpy_pad
On 10/26/2023 4:19 PM, Justin Stitt wrote:
strncpy() is deprecated for use on NUL-terminated destination strings
[1] and as such we should prefer more robust and less ambiguous string
interfaces.
`extra` is clearly supposed to be NUL-terminated which is evident by the
manual NUL-byte assignment as well as its immediate usage with strlen().
Moreover, let's NUL-pad since there is deliberate effort (48 instances)
made elsewhere to zero-out buffers in these getters and setters:
6050 | memset(local->config.nodeName, 0, sizeof(local->config.nodeName));
6130 | memset(local->config.rates, 0, 8);
6139 | memset(local->config.rates, 0, 8);
6414 | memset(key.key, 0, MAX_KEY_SIZE);
6497 | memset(extra, 0, 16);
(to be clear, strncpy also NUL-padded -- we are matching that behavior)
Considering the above, a suitable replacement is `strscpy_pad` due to
the fact that it guarantees both NUL-termination and NUL-padding on the
destination buffer.
We can also replace the hard-coded size of "16" to IW_ESSID_MAX_SIZE
because this function is a wext handler.
In wext-core.c we have:
static const struct iw_ioctl_description standard_ioctl[] = {
...
[IW_IOCTL_IDX(SIOCGIWNICKN)] = {
.header_type= IW_HEADER_TYPE_POINT,
.token_size = 1,
.max_tokens = IW_ESSID_MAX_SIZE,
},
So the buffer size is (strangely) IW_ESSID_MAX_SIZE
Link:
https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings
[1]
Link: https://github.com/KSPP/linux/issues/90
Cc: linux-hardening@vger.kernel.org
Signed-off-by: Justin Stitt
Reviewed-by: Jeff Johnson
Re: [PATCH v2] seq_buf: Introduce DECLARE_SEQ_BUF and seq_buf_str()
On Thu, Oct 26, 2023 at 12:40:37PM -0700, Kees Cook wrote: > Solve two ergonomic issues with struct seq_buf; > > 1) Too much boilerplate is required to initialize: > > struct seq_buf s; > char buf[32]; > > seq_buf_init(s, buf, sizeof(buf)); > > Instead, we can build this directly on the stack. Provide > DECLARE_SEQ_BUF() macro to do this: > > DECLARE_SEQ_BUF(s, 32); DECLARE_SEQ_BUF_ONSTACK maybe? But otherwise this looks like a good concept. > Instead, we can just return s->buffer direction after terminating it > in refactored seq_buf_terminate(), now known as seq_buf_str(): > > do_soemthing(seq_buf_str(s)); Looks good. Btw, one typical do_something would be printing it, so adding a format specifier that's using this helper would also probably be very useful.
