| 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
| ||
|
Message-ID: <i6is7g7gnyaczojw2wv7cw5sg7cqdafnzocfhu25tpbvccdbid@2k3q2r2qr47a>
Date: Wed, 30 Jul 2025 16:51:06 -0400
From: Seyediman Seyedarab <imandevel@...il.com>
To: dwmw2@...radead.org, baolu.lu@...ux.intel.com, joro@...tes.org,
will@...nel.org, robin.murphy@....com
Cc: iommu@...ts.linux.dev, linux-kernel@...r.kernel.org,
skhan@...uxfoundation.org, linux-kernel-mentees@...ts.linux.dev
Subject: Re: [PATCH v2] iommu/vt-d: replace snprintf with scnprintf in
dmar_latency_snapshot()
On 25/07/30 04:40PM, 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@...il.com>
> ---
> 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 | 8 ++------
> drivers/iommu/intel/perf.c | 8 ++++----
> drivers/iommu/intel/perf.h | 7 +++----
> 3 files changed, 9 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/iommu/intel/debugfs.c b/drivers/iommu/intel/debugfs.c
> index affbf4a1558d..c4aca0eb5e29 100644
> --- a/drivers/iommu/intel/debugfs.c
> +++ b/drivers/iommu/intel/debugfs.c
> @@ -653,12 +653,8 @@ static void latency_show_one(struct seq_file *m, struct intel_iommu *iommu,
> 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..7e726e1ba40b 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,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);
> }
> }
> diff --git a/drivers/iommu/intel/perf.h b/drivers/iommu/intel/perf.h
> index df9a36942d64..dcf2741c2d53 100644
> --- a/drivers/iommu/intel/perf.h
> +++ b/drivers/iommu/intel/perf.h
> @@ -35,12 +35,12 @@ struct latency_statistic {
> };
>
> #ifdef CONFIG_DMAR_PERF
> -int dmar_latency_enable(struct intel_iommu *iommu, enum latency_type type);
> +void dmar_latency_enable(struct intel_iommu *iommu, enum latency_type type);
> 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
>
My apologies, please don't apply this. I'll send a v3. I forgot to amend
the current patch... it changes the prototype of dmar_latency_enable():
> -int dmar_latency_enable(struct intel_iommu *iommu, enum latency_type type);
> +void dmar_latency_enable(struct intel_iommu *iommu, enum latency_type type);
Seyediman
Powered by blists - more mailing lists