netdev - Re: [PATCH net-next 00/15] tools: ynl: stop using libmnl

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] [day] [month] [year] [list]

Date: Tue, 27 Feb 2024 16:29:15 +0000
From: Donald Hunter <donald.hunter@...il.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: davem@...emloft.net,  netdev@...r.kernel.org,  edumazet@...gle.com,
  pabeni@...hat.com,  jiri@...nulli.us,  sdf@...gle.com,
  nicolas.dichtel@...nd.com
Subject: Re: [PATCH net-next 00/15] tools: ynl: stop using libmnl

Jakub Kicinski <kuba@...nel.org> writes:

> On Tue, 27 Feb 2024 10:49:42 +0000 Donald Hunter wrote:
>> > +static inline bool
>> > +__ynl_attr_put_overflow(struct nlmsghdr *nlh, size_t size)
>> > +{
>> > +	bool o;
>> > +
>> > +	/* We stash buffer length on nlmsg_pid. */
>> > +	o = nlh->nlmsg_len + NLA_HDRLEN + NLMSG_ALIGN(size) > nlh->nlmsg_pid;  
>> 
>> The comment confused me here. How about "We compare against stashed buffer
>> length in nlmsg_pid".
>
> The comment should give context, rather than describe the code so how
> about:
>
> 	/* ynl_msg_start() stashed buffer length in nlmsg_pid. */

LGTM.

>> > +	if (o)
>> > +		nlh->nlmsg_pid = YNL_MSG_OVERFLOW;  
>> 
>> It took me a moment to realise that this behaves like a very short
>> buffer length for subsequent calls to __ynl_attr_put_overflow(). Is it
>> worth extending the comment in ynl_msg_start() to say "buffer length or
>> overflow status"?
>
> Added:
> 		/* YNL_MSG_OVERFLOW is < NLMSG_HDRLEN, all subsequent checks
> 		 * are guaranteed to fail.
> 		 */
> SG?

Perfect.

>> > +	return o;
>> > +}
>> > +
>> >  static inline struct nlattr *
>> >  ynl_attr_nest_start(struct nlmsghdr *nlh, unsigned int attr_type)
>> >  {
>> >  	struct nlattr *attr;
>> >  
>> > +	if (__ynl_attr_put_overflow(nlh, 0))
>> > +		return ynl_nlmsg_end_addr(nlh) - NLA_HDRLEN;  
>> 
>> Is the idea here to return a struct nlattr * that is safe to use?
>> Shouldn't we zero the values in the buffer first?
>
> The only thing that the attr is used for is to call ynl_attr_nest_end().
> so I think zero init won't make any difference.

I guess it is fine for generated code.