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

Zahari Doychev
2025-10-22T20:00:30Z

On Tue, Oct 21, 2025 at 04:22:09PM -0700, Jakub Kicinski wrote:
> 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@linux.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);
>

thanks, we take a look at it.