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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20240227075605.18ec70b8@kernel.org>
Date: Tue, 27 Feb 2024 07:56:05 -0800
From: Jakub Kicinski <kuba@...nel.org>
To: Donald Hunter <donald.hunter@...il.com>
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

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

> > +	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?

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

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ