| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAGSyskWFhLKBE3f=rcPducGXwcUx8sFK5RuU-BLobV7wg8X8KA@mail.gmail.com>
Date: Fri, 7 Nov 2025 19:48:56 +0000
From: Gustavo Luiz Duarte <gustavold@...il.com>
To: Breno Leitao <leitao@...ian.org>
Cc: Andre Carvalho <asantostc@...il.com>, Simon Horman <horms@...nel.org>,
Andrew Lunn <andrew+netdev@...n.ch>, "David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
Shuah Khan <shuah@...nel.org>, netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-kselftest@...r.kernel.org
Subject: Re: [PATCH net-next 1/4] netconsole: Simplify send_fragmented_body()
On Fri, Nov 7, 2025 at 12:15 PM Breno Leitao <leitao@...ian.org> wrote:
> > diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
> > index 5d8d0214786c..0a8ba7c4bc9d 100644
> > --- a/drivers/net/netconsole.c
> > +++ b/drivers/net/netconsole.c
> > @@ -1553,13 +1553,16 @@ static void send_fragmented_body(struct netconsole_target *nt,
> > const char *msgbody, int header_len,
> > int msgbody_len, int extradata_len)
> > {
> > - int sent_extradata, preceding_bytes;
> > const char *extradata = NULL;
> > int body_len, offset = 0;
> > + int extradata_offset = 0;
> > + int msgbody_offset = 0;
> >
> > #ifdef CONFIG_NETCONSOLE_DYNAMIC
> > extradata = nt->extradata_complete;
> > #endif
>
>
> extradata could be NULL at this time if CONFIG_NETCONSOLE_DYNAMIC is
> unset. Basically extradata=NULL will not be replaced.
>
> > + if (WARN_ON_ONCE(!extradata && extradata_len != 0))
> > + return;
>
> And entradata_len = 0 for CONFIG_NETCONSOLE_DYNAMIC disabled.
>
> > + /* write msgbody first */
> > + this_chunk = min(msgbody_len - msgbody_offset,
> > + MAX_PRINT_CHUNK - this_header);
> > + memcpy(nt->buf + this_header, msgbody + msgbody_offset,
> > + this_chunk);
> > + msgbody_offset += this_chunk;
> > + this_offset += this_chunk;
> > +
> > + /* after msgbody, append extradata */
> > + this_chunk = min(extradata_len - extradata_offset,
> > + MAX_PRINT_CHUNK - this_header - this_offset);
> > + memcpy(nt->buf + this_header + this_offset,
> > + extradata + extradata_offset, this_chunk);
>
> then you are going to memcpy from NULL pointer (`extradata + extradata_offset` == 0).
I believe passing NULL to memcpy() should be safe as long as count is
zero (which is the case here).
However, what I didn't realize at first is that we would be doing
pointer arithmetic with NULL, which is undefined behavior :(
I will add a check if extradata is NULL here.
Thanks for the careful review!
>
> I got this my vim LSP that printed:
>
> Null pointer passed as 2nd argument to memory copy function [unix.cstring.NullArg]
>
On Fri, Nov 7, 2025 at 12:15 PM Breno Leitao <leitao@...ian.org> wrote:
>
> On Wed, Nov 05, 2025 at 09:06:43AM -0800, Gustavo Luiz Duarte wrote:
> > Refactor send_fragmented_body() to use separate offset tracking for
> > msgbody, and extradata instead of complex conditional logic.
> > The previous implementation used boolean flags and calculated offsets
> > which made the code harder to follow.
> >
> > The new implementation maintains independent offset counters
> > (msgbody_offset, extradata_offset) and processes each section
> > sequentially, making the data flow more straightforward and the code
> > easier to maintain.
> >
> > This is a preparatory refactoring with no functional changes, which will
> > allow easily splitting extradata_complete into separate userdata and
> > sysdata buffers in the next patch.
> >
> > Signed-off-by: Gustavo Luiz Duarte <gustavold@...il.com>
> > ---
> > drivers/net/netconsole.c | 73 ++++++++++++++++--------------------------------
> > 1 file changed, 24 insertions(+), 49 deletions(-)
> >
> > diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
> > index 5d8d0214786c..0a8ba7c4bc9d 100644
> > --- a/drivers/net/netconsole.c
> > +++ b/drivers/net/netconsole.c
> > @@ -1553,13 +1553,16 @@ static void send_fragmented_body(struct netconsole_target *nt,
> > const char *msgbody, int header_len,
> > int msgbody_len, int extradata_len)
> > {
> > - int sent_extradata, preceding_bytes;
> > const char *extradata = NULL;
> > int body_len, offset = 0;
> > + int extradata_offset = 0;
> > + int msgbody_offset = 0;
> >
> > #ifdef CONFIG_NETCONSOLE_DYNAMIC
> > extradata = nt->extradata_complete;
> > #endif
>
>
> extradata could be NULL at this time if CONFIG_NETCONSOLE_DYNAMIC is
> unset. Basically extradata=NULL will not be replaced.
>
> > + if (WARN_ON_ONCE(!extradata && extradata_len != 0))
> > + return;
>
> And entradata_len = 0 for CONFIG_NETCONSOLE_DYNAMIC disabled.
>
> > + /* write msgbody first */
> > + this_chunk = min(msgbody_len - msgbody_offset,
> > + MAX_PRINT_CHUNK - this_header);
> > + memcpy(nt->buf + this_header, msgbody + msgbody_offset,
> > + this_chunk);
> > + msgbody_offset += this_chunk;
> > + this_offset += this_chunk;
> > +
> > + /* after msgbody, append extradata */
> > + this_chunk = min(extradata_len - extradata_offset,
> > + MAX_PRINT_CHUNK - this_header - this_offset);
> > + memcpy(nt->buf + this_header + this_offset,
> > + extradata + extradata_offset, this_chunk);
>
> then you are going to memcpy from NULL pointer (`extradata + extradata_offset` == 0).
>
> I got this my vim LSP that printed:
>
> Null pointer passed as 2nd argument to memory copy function [unix.cstring.NullArg]
>
Powered by blists - more mailing lists