Re: [PATCH 03/15] tools: ynl: Teach ynl_gen_c to validate and dump 'blob' attributes

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] The generated `_more()` function fails to coalesce blob chunks if the length attribute arrives in a message prior to the blob data, breaking multi-message coalescing.
- [High] The generated C setter function for blobs lacks a NULL check for `malloc` and is vulnerable to integer overflow on 32-bit systems, causing memory corruption or crashes.
- [High] Request serialization (`attr_put`) does not support chunking large blobs, leading to silent data corruption due to 16-bit `nla_len` truncation if the blob exceeds 64KB.
- [Medium] `ynl_exec` fails to check for incomplete blobs, leading to silent data truncation for `do` commands.
--

commit 5e458f41fb05443af96a2ab1a2945032cdac2de5
Author: Dan Williams <djbw@kernel.org>

tools: ynl: Teach ynl_gen_c to validate and dump 'blob' attributes

Now that blobs are recognized by pyynl and coalesced over successive
netlink messages, do the same for the generated C. Recall that blobs
support large objects for protocols like SPDM.

Link: http://lore.kernel.org/20260318170014.6650d2bf@kernel.org [1]

> diff --git a/tools/net/ynl/lib/ynl.c b/tools/net/ynl/lib/ynl.c
> index 2bcd781111d74..933ee0ad2c51d 100644
> --- a/tools/net/ynl/lib/ynl.c
> +++ b/tools/net/ynl/lib/ynl.c
[ ... ]
> @@ -1059,6 +1087,11 @@ int ynl_exec_dump(struct ynl_sock *ys, struct nlmsghdr *req_nlh,
>  			goto err_close_list;
>  	} while (err > 0);
>  
> +	if (ynl_check_more(yds)) {
> +		yerr(ys, YNL_ERROR_ATTR_MISSING, "Dump ended with an incomplete blob");
> +		goto err_close_list;
> +	}

[Severity: Medium]
While ynl_exec_dump() verifies that the blob is complete here, does ynl_exec()
for single command responses completely omit this validation?

If the kernel returns a response where the provided blob data is shorter
than the advertised length, will the parser silently succeed and leave
userspace with a truncated blob?

Should struct ynl_req_state also include a more callback?

> diff --git a/tools/net/ynl/pyynl/ynl_gen_c.py b/tools/net/ynl/pyynl/ynl_gen_c.py
> index cdc3646f2642c..f0387004b7ccf 100755
> --- a/tools/net/ynl/pyynl/ynl_gen_c.py
> +++ b/tools/net/ynl/pyynl/ynl_gen_c.py
[ ... ]
> @@ -791,6 +818,10 @@ class TypeMultiAttr(Type):
>              put_type = self.type
>              ri.cw.p(f"for (i = 0; i < {var}->_count.{self.c_name}; i++)")
>              ri.cw.p(f"ynl_attr_put_{put_type}(nlh, {self.enum_name}, {var}->{self.c_name}[i]);")
> +        elif self.attr['type'] == 'binary' and self.is_blob():
> +            ri.cw.p(f"if ({var}->{self.c_name})")
> +            ri.cw.p(f"ynl_attr_put(nlh, {self.enum_name}, " +
> +                    f"{var}->{self.c_name}->data, {var}->{self.c_name}->len);")

[Severity: High]
Does ynl_attr_put() support chunking blobs larger than 64KB?

If a user application attempts to send a blob larger than 65531 bytes, it
appears nla_len will overflow its 16-bit capacity in ynl_attr_put():

    attr->nla_len = NLA_HDRLEN + size;
    memcpy(ynl_attr_data(attr), value, size);
    nlh->nlmsg_len += NLMSG_ALIGN(attr->nla_len);

Will nlmsg_len then advance incorrectly while memcpy() copies the full data,
causing subsequent attributes to overwrite the middle of the blob?

[ ... ]
> @@ -805,6 +836,10 @@ class TypeMultiAttr(Type):
>              raise Exception(f"Put of MultiAttr sub-type {self.attr['type']} not supported yet")
>  
>      def _setter_lines(self, ri, member, presence):
> +        if self.is_blob():
> +            return [f"{presence} = {self.c_name}->len;",
> +                    f"{member} = malloc(sizeof(*{member}) + {presence});",
> +                    f"memcpy({member}, {self.c_name}, sizeof(*{member}) + {presence});"]

[Severity: High]
Does this generated C code lack a NULL check for the malloc() return value?

If malloc() fails, the returned NULL pointer will be passed directly to
memcpy(), potentially causing a crash.

Additionally, on 32-bit systems, could sizeof(*member) + presence result in an
integer overflow if the length is large? If the addition wraps around, it
might allocate a small buffer while leaving the length field corrupted.

[ ... ]
> @@ -2388,6 +2424,33 @@ def parse_rsp_msg(ri, deref=False):
>          ri.cw.nl()
>  
>  
> +def _blob_attrs(ri):
> +    return [attr for _, attr in ri.struct['reply'].member_list() if attr.bloblen]
> +
> +
> +def print_rsp_more(ri):
> +    blob_attrs = _blob_attrs(ri)
> +    if not blob_attrs:
> +        return
> +
> +    local_vars = [f'{type_name(ri, "reply", deref=True)} *rsp = data;']
> +
> +    ri.cw.write_func_prot('static bool',
> +                          f'{op_prefix(ri, "reply", deref=True)}_more',
> +                          ['void *data'], suffix='')
> +    ri.cw.block_start()
> +    ri.cw.write_func_lvar(local_vars)
> +
> +    for attr in blob_attrs:
> +        ri.cw.p(f"if (rsp->{attr.c_name} && " +
> +                f"rsp->_len.{attr.c_name} < rsp->{attr.c_name}->len)")
> +        ri.cw.p('return true;')

[Severity: High]
If the blob length attribute arrives in a netlink message prior to the blob
data message, will the blob pointer still be NULL?

If the blob is NULL, this function will return false. Would that cause
ynl_dump_trampoline() to fall through and allocate a new object instead of
coalescing the data, dropping the previously parsed length?

When the next message with the data chunk arrives, wouldn't it allocate a
0-length blob and fail with a "blob data exceeds length" error?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260705220819.2472765-1-djbw@kernel.org?part=3