tools: ynl: Fix tc filters with actions

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

Posted by Zahari Doychev 3 months, 3 weeks ago

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.

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);
 
 	ys->family = yf;
 	ys->tx_buf = &ys->raw_buf[0];
-- 
2.51.0

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

Posted by Jakub Kicinski 3 months, 2 weeks ago

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?

> 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.

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

Posted by Zahari Doychev 3 months, 2 weeks ago

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?

Thanks

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

Posted by Jakub Kicinski 3 months, 2 weeks ago

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);

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

Posted by Zahari Doychev 3 months, 2 weeks ago

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.