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

[Google]

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.

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


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