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

[Josh Law]

On 17 March 2026 23:15:40 GMT, Masami Hiramatsu <mhiramat@xxxxxxxxxx> wrote:
>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
>> 
>
>


I submitted V7 dropping all them patches anyway, V7 should be perfect now.


V/R


Josh Law