Re: [PATCH v4 0/2] lib/vsprintf: Fixes size check

[David Laight]

On Fri, 27 Mar 2026 16:28:12 +0900
Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx> wrote:

> On Thu, 26 Mar 2026 09:12:12 +0000
> David Laight <david.laight.linux@xxxxxxxxx> wrote:
> 
> > On Thu, 26 Mar 2026 16:39:44 +0900
> > Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx> wrote:
> >   
> > > On Wed, 25 Mar 2026 10:20:39 +0000
> > > David Laight <david.laight.linux@xxxxxxxxx> wrote:
> > >   
> > > > On Tue, 24 Mar 2026 22:04:58 -0700
> > > > Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> wrote:
> > > >     
> > > > > On Wed, 25 Mar 2026 11:25:06 +0900 "Masami Hiramatsu (Google)" <mhiramat@xxxxxxxxxx> wrote:
> > > > >     
> > > > > > Here is the 4th version of patches to fix vsnprintf().
> > > > > > 
> > > > > >  - Fix to limit the size of width and precision.
> > > > > >  - Warn if the return size is over INT_MAX.
> > > > > > 
> > > > > > Previous version is here;
> > > > > > 
> > > > > > https://lore.kernel.org/all/177410406326.38798.16853803119128725972.stgit@devnote2/
> > > > > > 
> > > > > > In this version, do clamp() the width and precision before checking it and
> > > > > > accept negative precision[1/3] and add Petr's Reviewed-by[2/2].      
> > > > > 
> > > > > AI review has flagged a couple of possible issues:
> > > > > 	https://sashiko.dev/#/patchset/177440550682.147866.1854734911195480940.stgit@devnote2    
> > > > 
> > > > I'd guess there are exactly 0 places where a negative precision is passed
> > > > to "%.*s" - if there were any someone would have complained about the
> > > > output being missing.
> > > > Checking all 759 cases grep -r '".*%.*\.%*s.*"' found will be tedious.
> > > > But pretty much all are 'namelen'.    
> > > 
> > > I also verified and found only one suspicious usage which can pass
> > > a negative precision.  
> > 
> > It is always called with a constant, in any case the string being output
> > is constant so nothing nasty can happen.  
> 
> Since this function is not a static function, it can be called anywhere in
> the module. So it is safer to check the indent is positive.

That would have to be an out or tree module using a function that looks
pretty specific to the i915 display code.
Even as someone who has supported out of tree drivers I'd say that they
get what they deserve.
The snprintf() code itself won't break anything.

	David
 
> 
> > I didn't even see any recursive/loop calls that indent by significant amounts.
> > The code could use the more usual ("%*s", indent, ""), but it doesn't matter
> > much - mostly just a shorter line.  
> 
> I think the current code looks a bit tricky and not clear what it does.
> Maybe it is better to replace it with above usual usage.
> 
> Thanks,
> 
> > 
> > 	David
> >   
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
> > > index d2c7b1090df0..1f90775ea8a8 100644
> > > --- a/drivers/gpu/drm/i915/i915_request.c
> > > +++ b/drivers/gpu/drm/i915/i915_request.c
> > > @@ -2224,7 +2224,7 @@ void i915_request_show(struct drm_printer *m,
> > >  	rcu_read_lock();
> > >  	timeline = dma_fence_timeline_name((struct dma_fence *)&rq->fence);
> > >  	drm_printf(m, "%s%.*s%c %llx:%lld%s%s %s @ %dms: %s\n",
> > > -		   prefix, indent, "                ",
> > > +		   prefix, max(0, indent), "                ",
> > >  		   queue_status(rq),
> > >  		   rq->fence.context, rq->fence.seqno,
> > >  		   run_status(rq),
> > > 
> > > Thanks,  
> > > > 
> > > > In any case worst thing should be a panic if the code hits an invalid
> > > > address before finding a '\0' byte - probably unlikely anyway.
> > > > 
> > > > I'd fix it, but try to stop it being backported.
> > > > 
> > > > 	David    
> > > 
> > >   
> >   
> 
>