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

[Baolu Lu <baolu.lu@linux.intel.com>]

On 8/7/25 17:00, Seyediman Seyedarab wrote:
> On 25/07/31 06:50PM, Seyediman Seyedarab wrote:
>> snprintf() returns the number of bytes that would have been
>> written, not the number actually written. Using this for offset
>> tracking can cause buffer overruns if truncation occurs.
>>
>> Replace snprintf() with scnprintf() to ensure the offset stays
>> within bounds.
>>
>> Since scnprintf() never returns a negative value, and zero is not
>> possible in this context because 'bytes' starts at 0 and 'size - bytes'
>> is DEBUG_BUFFER_SIZE in the first call, which is large enough to hold
>> the string literals used, the return value is always positive. An integer
>> overflow is also completely out of reach here due to the small and fixed
>> buffer size. The error check in latency_show_one() is therefore unnecessary.
>> Remove it and make dmar_latency_snapshot() return void.
>>
>> Signed-off-by: Seyediman Seyedarab<ImanDevel@gmail.com>
>> ---
>> Changes in v4:
>> - Removed 'ret' in latency_show_one() since it is not being used anymore:
>> https://lore.kernel.org/oe-kbuild-all/202508010632.WB0CM5Bz-lkp@intel.com/
>>
>> Changes in v3:
>> - Restored return type of dmar_latency_enable() back to 'int'. It was
>>    mistakenly changed to 'void' in the previous version.
>>
>> Changes in v2:
>> - The return type of dmar_latency_snapshot() was changed based on the
>>    discussion here:
>> https://lore.kernel.org/linux-iommu/aIDN3pvUSG3rN4SW@willie-the-truck/
>>
>>
>>   drivers/iommu/intel/debugfs.c | 10 ++--------
>>   drivers/iommu/intel/perf.c    | 10 ++++------
>>   drivers/iommu/intel/perf.h    |  5 ++---
>>   3 files changed, 8 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/iommu/intel/debugfs.c b/drivers/iommu/intel/debugfs.c
>> index affbf4a1558d..65d2f792f0f7 100644
>> --- a/drivers/iommu/intel/debugfs.c
>> +++ b/drivers/iommu/intel/debugfs.c
>> @@ -648,17 +648,11 @@ DEFINE_SHOW_ATTRIBUTE(ir_translation_struct);
>>   static void latency_show_one(struct seq_file *m, struct intel_iommu *iommu,
>>   			     struct dmar_drhd_unit *drhd)
>>   {
>> -	int ret;
>> -
>>   	seq_printf(m, "IOMMU: %s Register Base Address: %llx\n",
>>   		   iommu->name, drhd->reg_base_addr);
>>   
>> -	ret = dmar_latency_snapshot(iommu, debug_buf, DEBUG_BUFFER_SIZE);
>> -	if (ret < 0)
>> -		seq_puts(m, "Failed to get latency snapshot");
>> -	else
>> -		seq_puts(m, debug_buf);
>> -	seq_puts(m, "\n");
>> +	dmar_latency_snapshot(iommu, debug_buf, DEBUG_BUFFER_SIZE);
>> +	seq_printf(m, "%s\n", debug_buf);
>>   }
>>   
>>   static int latency_show(struct seq_file *m, void *v)
>> diff --git a/drivers/iommu/intel/perf.c b/drivers/iommu/intel/perf.c
>> index adc4de6bbd88..dceeadc3ee7c 100644
>> --- a/drivers/iommu/intel/perf.c
>> +++ b/drivers/iommu/intel/perf.c
>> @@ -113,7 +113,7 @@ static char *latency_type_names[] = {
>>   	"     svm_prq"
>>   };
>>   
>> -int dmar_latency_snapshot(struct intel_iommu *iommu, char *str, size_t size)
>> +void dmar_latency_snapshot(struct intel_iommu *iommu, char *str, size_t size)
>>   {
>>   	struct latency_statistic *lstat = iommu->perf_statistic;
>>   	unsigned long flags;
>> @@ -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,11 +156,9 @@ 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);
>>   		}
>>   	}
>>   	spin_unlock_irqrestore(&latency_lock, flags);
>> -
>> -	return bytes;
>>   }
>> diff --git a/drivers/iommu/intel/perf.h b/drivers/iommu/intel/perf.h
>> index df9a36942d64..1d4baad7e852 100644
>> --- a/drivers/iommu/intel/perf.h
>> +++ b/drivers/iommu/intel/perf.h
>> @@ -40,7 +40,7 @@ void dmar_latency_disable(struct intel_iommu *iommu, enum latency_type type);
>>   bool dmar_latency_enabled(struct intel_iommu *iommu, enum latency_type type);
>>   void dmar_latency_update(struct intel_iommu *iommu, enum latency_type type,
>>   			 u64 latency);
>> -int dmar_latency_snapshot(struct intel_iommu *iommu, char *str, size_t size);
>> +void dmar_latency_snapshot(struct intel_iommu *iommu, char *str, size_t size);
>>   #else
>>   static inline int
>>   dmar_latency_enable(struct intel_iommu *iommu, enum latency_type type)
>> @@ -64,9 +64,8 @@ dmar_latency_update(struct intel_iommu *iommu, enum latency_type type, u64 laten
>>   {
>>   }
>>   
>> -static inline int
>> +static inline void
>>   dmar_latency_snapshot(struct intel_iommu *iommu, char *str, size_t size)
>>   {
>> -	return 0;
>>   }
>>   #endif /* CONFIG_DMAR_PERF */
>> -- 
>> 2.50.1
>>
> Hi there,
> 
> Just following up on this patch. Please let me know if there's
> anything else needed from my side.

It just missed the v6.17 merge window, I'll queue it for v6.18.

Thanks,
baolu