Re: [PATCH v6 16/17] lib/bootconfig: fix sign-compare in xbc_node_compose_key_after()

[Google]

On Tue, 17 Mar 2026 12:15:07 -0400
Steven Rostedt <rostedt@xxxxxxxxxxx> wrote:

> On Tue, 17 Mar 2026 16:55:49 +0900
> Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx> wrote:
> 
> > > --- a/lib/bootconfig.c
> > > +++ b/lib/bootconfig.c
> > > @@ -319,10 +319,10 @@ int __init xbc_node_compose_key_after(struct xbc_node *root,
> > >  			       depth ? "." : "");
> > >  		if (ret < 0)
> > >  			return ret;
> > > -		if (ret >= size) {
> > > +		if (ret >= (int)size) {  
> > 
> > nit:
> > 
> > 	if ((size_t)ret >= size) {
> > 
> > because sizeof(size_t) > sizeof(int).
> 
> I don't think we need to worry about this. But this does bring up an issue.
> ret comes from:
> 
> 		ret = snprintf(buf, size, "%s%s", xbc_node_get_data(node),
> 			       depth ? "." : "");
> 
> Where size is of type size_t
> 
> snprintf() takes size_t but returns int.
> 
> snprintf() calls vsnprintf() which has:
> 
> 	size_t len, pos;
> 
> Where pos is incremented based on fmt, and vsnprintf() returns:
> 
> 	return pos;
> 
> Which can overflow.

I think that is vsnprintf() (maybe POSIX) design issue.
I believe we're simply using the size_t to represent size of memory
out of convention.

> 
> Now, honestly, we should never have a 2Gig string as that would likely
> cause other horrible things. Does size really need to be size_t?

Even if so, it should be done in vsnprintf() instead of this.
This function just believes that the caller gives collect size
and enough amount of memory. Or, we need to check "INT_MAX > size"
in everywhere.

> 
> Perhaps we should have:
> 
> 	if (WARN_ON_ONCE(size > MAX_INT))
> 		return -EINVAL;

I think this is an over engineering effort especially in
caller side. This overflow should be checked in vsnprintf() and
should return -EINVAL. (and the caller checks the return value.)

Thank you,

> 
> ?
> 
> -- Steve
> 


-- 
Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx>