linux-kernel - Re: [PATCH] iommu/vt-d: replace snprintf with scnprintf in dmar_latency

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <fvq3h4mbms64vzyqssy4xli2sudzpyimbacg74lkdgrzi77oqy@4yywt5fav7wi>
Date: Wed, 23 Jul 2025 04:28:54 -0400
From: Seyediman Seyedarab <imandevel@...il.com>
To: Will Deacon <will@...nel.org>
Cc: dwmw2@...radead.org, baolu.lu@...ux.intel.com, joro@...tes.org, 
	robin.murphy@....com, skhan@...uxfoundation.org, linux-kernel-mentees@...ts.linux.dev, 
	iommu@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] iommu/vt-d: replace snprintf with scnprintf in
 dmar_latency_snapshot()

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@...il.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