Re: [PATCH] iommu/vt-d: replace snprintf with scnprintf in dmar_latency_snapshot()

[Seyediman Seyedarab]

On 25/07/23 12:56PM, Will Deacon wrote:
> On Wed, Jul 23, 2025 at 04:28:54AM -0400, Seyediman Seyedarab wrote:
> > On 25/07/22 06:58PM, Will Deacon wrote:
> > > On Tue, Jul 22, 2025 at 09:11:17AM -0400, Seyediman Seyedarab wrote:
> > > > snprintf returns the number of bytes that would have been written,
> > > > not the number actually written to the buffer. When accumulating
> > > > the byte count with the return value of snprintf, this can cause
> > > > the offset to exceed the actual buffer size if truncation occurs.
> > > > 
> > > > The byte count is passed to seq_puts() in latency_show_one() with-
> > > > out checking for truncation.
> > > > 
> > > > Replace snprintf with scnprintf, ensuring the buffer offset stays
> > > > within bound.
> > > > 
> > > > Signed-off-by: Seyediman Seyedarab <ImanDevel@xxxxxxxxx>
> > > > ---
> > > >  drivers/iommu/intel/perf.c | 6 +++---
> > > >  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> [...]
> 
> > > Should the check of the return value in latency_show_one() also be
> > > adjusted so that 'ret <= 0' is an error? I couldn't convince myself
> > > that the string in 'debug_buf' is always null-terminated if ret == 0.
> > > 
> > IMO, that's not necessary. 'bytes' can't be less than zero that's
> > for sure (AFAIK, scnprintf() doesn't have any case where it returns
> > a negative number).
> > As for being zero, in every scnprintf() call, 'size - bytes' would
> > have to be == 0. (or size > INT_MAX, but still you get zero, not a
> > negative number as an error)
> > 
> > In latency_show_one(), the 'size' is DEBUG_BUFFER_SIZE, and
> > 'bytes' in the first run is 0. So, 'size - bytes' == DEBUG_BUFFER_SIZE.
> > Since 'latency_counter_names' and 'latency_type_names' are arrays of
> > string literals, 'bytes' is guaranteed to be increased in the first
> > iteration, even if the rest become zero (which won't happen, since
> > they are smaller than DEBUG_BUFFER_SIZE).
> > 
> > So, the case of zero is impossible, unless you want a bulletproof
> > check for future implementations where the function might be rewritten.
> 
> Thanks, so that sounds like the error check in latency_show_one() is dead
> code in which case dmar_latency_snapshot() should just have a return type
> of 'void'?
> 
> Will

Well, there’s only one case where dmar_latency_snapshot() could go
wrong and return a negative number, and that's due to integer overflow.
It seems unlikely and probably out of reach, but increasing the number
of string literals might trigger it. So, I wouldn't say it's entirely
dead code. We can change it to 'if (unlikely(ret < 0))', or if it's
too out of reach I can send another patch to remove it and change
dmar_latency_snapshot()'s return type to void. (or a v2 including
both changes)

Seyediman