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@gmail.com>
---
 drivers/iommu/intel/perf.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/iommu/intel/perf.c b/drivers/iommu/intel/perf.c
index adc4de6bb..cee4821f4 100644
--- a/drivers/iommu/intel/perf.c
+++ b/drivers/iommu/intel/perf.c
@@ -122,7 +122,7 @@ int dmar_latency_snapshot(struct intel_iommu *iommu, char *str, size_t size)
 	memset(str, 0, size);
 
 	for (i = 0; i < COUNTS_NUM; i++)
-		bytes += snprintf(str + bytes, size - bytes,
+		bytes += scnprintf(str + bytes, size - bytes,
 				  "%s", latency_counter_names[i]);
 
 	spin_lock_irqsave(&latency_lock, flags);
@@ -130,7 +130,7 @@ int dmar_latency_snapshot(struct intel_iommu *iommu, char *str, size_t size)
 		if (!dmar_latency_enabled(iommu, i))
 			continue;
 
-		bytes += snprintf(str + bytes, size - bytes,
+		bytes += scnprintf(str + bytes, size - bytes,
 				  "\n%s", latency_type_names[i]);
 
 		for (j = 0; j < COUNTS_NUM; j++) {
@@ -156,7 +156,7 @@ int dmar_latency_snapshot(struct intel_iommu *iommu, char *str, size_t size)
 				break;
 			}
 
-			bytes += snprintf(str + bytes, size - bytes,
+			bytes += scnprintf(str + bytes, size - bytes,
 					  "%12lld", val);
 		}
 	}
-- 
2.50.1

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@gmail.com>
> ---
>  drivers/iommu/intel/perf.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iommu/intel/perf.c b/drivers/iommu/intel/perf.c
> index adc4de6bb..cee4821f4 100644
> --- a/drivers/iommu/intel/perf.c
> +++ b/drivers/iommu/intel/perf.c
> @@ -122,7 +122,7 @@ int dmar_latency_snapshot(struct intel_iommu *iommu, char *str, size_t size)
>  	memset(str, 0, size);
>  
>  	for (i = 0; i < COUNTS_NUM; i++)
> -		bytes += snprintf(str + bytes, size - bytes,
> +		bytes += scnprintf(str + bytes, size - bytes,
>  				  "%s", latency_counter_names[i]);
>  
>  	spin_lock_irqsave(&latency_lock, flags);
> @@ -130,7 +130,7 @@ int dmar_latency_snapshot(struct intel_iommu *iommu, char *str, size_t size)
>  		if (!dmar_latency_enabled(iommu, i))
>  			continue;
>  
> -		bytes += snprintf(str + bytes, size - bytes,
> +		bytes += scnprintf(str + bytes, size - bytes,
>  				  "\n%s", latency_type_names[i]);
>  
>  		for (j = 0; j < COUNTS_NUM; j++) {
> @@ -156,7 +156,7 @@ int dmar_latency_snapshot(struct intel_iommu *iommu, char *str, size_t size)
>  				break;
>  			}
>  
> -			bytes += snprintf(str + bytes, size - bytes,
> +			bytes += scnprintf(str + bytes, size - bytes,
>  					  "%12lld", val);

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.

Will

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@gmail.com>
> > ---
> >  drivers/iommu/intel/perf.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/iommu/intel/perf.c b/drivers/iommu/intel/perf.c
> > index adc4de6bb..cee4821f4 100644
> > --- a/drivers/iommu/intel/perf.c
> > +++ b/drivers/iommu/intel/perf.c
> > @@ -122,7 +122,7 @@ int dmar_latency_snapshot(struct intel_iommu *iommu, char *str, size_t size)
> >  	memset(str, 0, size);
> >  
> >  	for (i = 0; i < COUNTS_NUM; i++)
> > -		bytes += snprintf(str + bytes, size - bytes,
> > +		bytes += scnprintf(str + bytes, size - bytes,
> >  				  "%s", latency_counter_names[i]);
> >  
> >  	spin_lock_irqsave(&latency_lock, flags);
> > @@ -130,7 +130,7 @@ int dmar_latency_snapshot(struct intel_iommu *iommu, char *str, size_t size)
> >  		if (!dmar_latency_enabled(iommu, i))
> >  			continue;
> >  
> > -		bytes += snprintf(str + bytes, size - bytes,
> > +		bytes += scnprintf(str + bytes, size - bytes,
> >  				  "\n%s", latency_type_names[i]);
> >  
> >  		for (j = 0; j < COUNTS_NUM; j++) {
> > @@ -156,7 +156,7 @@ int dmar_latency_snapshot(struct intel_iommu *iommu, char *str, size_t size)
> >  				break;
> >  			}
> >  
> > -			bytes += snprintf(str + bytes, size - bytes,
> > +			bytes += scnprintf(str + bytes, size - bytes,
> >  					  "%12lld", val);
> 
> 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.
> 
> Will

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.

Seyediman

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@gmail.com>
> > > ---
> > >  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

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@gmail.com>
> > > > ---
> > > >  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