netdev - Re: [PATCH 2/4] tools: ynl: zero-initialize struct ynl

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20251021162209.73215f57@kernel.org>
Date: Tue, 21 Oct 2025 16:22:09 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: Zahari Doychev <zahari.doychev@...ux.com>
Cc: donald.hunter@...il.com, davem@...emloft.net, edumazet@...gle.com,
 pabeni@...hat.com, horms@...nel.org, jacob.e.keller@...el.com,
 ast@...erby.net, matttbe@...nel.org, netdev@...r.kernel.org,
 linux-kernel@...r.kernel.org, jhs@...atatu.com, xiyou.wangcong@...il.com,
 jiri@...nulli.us, johannes@...solutions.net
Subject: Re: [PATCH 2/4] tools: ynl: zero-initialize struct ynl_sock memory

On Tue, 21 Oct 2025 20:36:38 +0300 Zahari Doychev wrote:
> On Mon, Oct 20, 2025 at 04:16:39PM -0700, Jakub Kicinski wrote:
> > On Sat, 18 Oct 2025 17:17:35 +0200 Zahari Doychev wrote:  
> > > The memory belonging to tx_buf and rx_buf in ynl_sock is not
> > > initialized after allocation. This commit ensures the entire
> > > allocated memory is set to zero.
> > > 
> > > When asan is enabled, uninitialized bytes may contain poison values.
> > > This can cause failures e.g. when doing ynl_attr_put_str then poisoned
> > > bytes appear after the null terminator. As a result, tc filter addition
> > > may fail.  
> > 
> > We add strings with the null-terminating char, AFAICT.
> > Do you mean that the poison value appears in the padding?
> >   
> 
> Yes, correct. The function nla_strcmp(...) does not match in this case as
> the poison value appears in the padding after the null byte.
> 
> > > Signed-off-by: Zahari Doychev <zahari.doychev@...ux.com>
> > > ---
> > >  tools/net/ynl/lib/ynl.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/tools/net/ynl/lib/ynl.c b/tools/net/ynl/lib/ynl.c
> > > index 2bcd781111d7..16a4815d6a49 100644
> > > --- a/tools/net/ynl/lib/ynl.c
> > > +++ b/tools/net/ynl/lib/ynl.c
> > > @@ -744,7 +744,7 @@ ynl_sock_create(const struct ynl_family *yf, struct ynl_error *yse)
> > >  	ys = malloc(sizeof(*ys) + 2 * YNL_SOCKET_BUFFER_SIZE);
> > >  	if (!ys)
> > >  		return NULL;
> > > -	memset(ys, 0, sizeof(*ys));
> > > +	memset(ys, 0, sizeof(*ys) + 2 * YNL_SOCKET_BUFFER_SIZE);  
> > 
> > This is just clearing the buffer initially, it can be used for multiple
> > requests. This change is no good as is.  
> 
> I see. Should then the ynl_attr_put_str be changed to zero the padding
> bytes or it is better to make sure the buffers are cleared for each
> request?

Eek, I think the bug is in how ynl_attr_put_str() computes len.
len is attr len, it should not include padding.
At the same time we should probably zero-terminate the strings
in case kernel wants NLA_NUL_STRING.

Just for illustration -- I think we should do something like 
the following, please turn this into a real patch if it makes sense:

diff --git a/tools/net/ynl/lib/ynl-priv.h b/tools/net/ynl/lib/ynl-priv.h
index 29481989ea76..515c6d12f68a 100644
--- a/tools/net/ynl/lib/ynl-priv.h
+++ b/tools/net/ynl/lib/ynl-priv.h
@@ -314,14 +314,14 @@ ynl_attr_put_str(struct nlmsghdr *nlh, unsigned int attr_type, const char *str)
        size_t len;
 
        len = strlen(str);
-       if (__ynl_attr_put_overflow(nlh, len))
+       if (__ynl_attr_put_overflow(nlh, len + 1))
                return;
 
        attr = (struct nlattr *)ynl_nlmsg_end_addr(nlh);
        attr->nla_type = attr_type;
 
        strcpy((char *)ynl_attr_data(attr), str);
-       attr->nla_len = NLA_HDRLEN + NLA_ALIGN(len);
+       attr->nla_len = NLA_HDRLEN + len + 1;
 
        nlh->nlmsg_len += NLMSG_ALIGN(attr->nla_len);