Re: [PATCH v1] ptp: ocp: Limit SMA/signal/freq counts in show/store functions

[Simon Horman <horms@kernel.org>]

On Tue, May 06, 2025 at 11:06:47AM +0300, Sagi Maimon wrote:
> The sysfs show/store operations could access uninitialized elements in
> the freq_in[], signal_out[], and sma[] arrays, leading to NULL pointer
> dereferences. This patch introduces u8 fields (nr_freq_in, nr_signal_out,
> nr_sma) to track the actual number of initialized elements, capping the
> maximum at 4 for each array. The affected show/store functions are updated to
> respect these limits, preventing out-of-bounds access and ensuring safe
> array handling.
> 
> Signed-off-by: Sagi Maimon <maimon.sagi@gmail.com>

Hi Sagi,

With this patch applied GCC 14.2.0 reports:

  .../ptp_ocp.c: In function 'ptp_ocp_summary_show':
  .../ptp_ocp.c:4052:28: warning: '%d' directive writing between 1 and 11 bytes into a region of size 5 [-Wformat-overflow=]
   4052 |         sprintf(label, "GEN%d", nr + 1);
        |                            ^~
  In function '_signal_summary_show',
      inlined from 'ptp_ocp_summary_show' at drivers/ptp/ptp_ocp.c:4215:4:
  .../ptp_ocp.c:4052:24: note: directive argument in the range [-2147483639, 2147483647]
   4052 |         sprintf(label, "GEN%d", nr + 1);
        |                        ^~~~~~~
  .../ptp_ocp.c:4052:9: note: 'sprintf' output between 5 and 15 bytes into a destination of size 8
   4052 |         sprintf(label, "GEN%d", nr + 1);
        |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  .../ptp_ocp.c: In function 'ptp_ocp_summary_show':
  .../ptp_ocp.c:4077:29: warning: '%d' directive writing between 1 and 11 bytes into a region of size 4 [-Wformat-overflow=]
   4077 |         sprintf(label, "FREQ%d", nr + 1);
        |                             ^~
  In function '_frequency_summary_show',
      inlined from 'ptp_ocp_summary_show' at drivers/ptp/ptp_ocp.c:4219:4:
  .../ptp_ocp.c:4077:24: note: directive argument in the range [-2147483640, 2147483647]
   4077 |         sprintf(label, "FREQ%d", nr + 1);
        |                        ^~~~~~~~
  .../ptp_ocp.c:4077:9: note: 'sprintf' output between 6 and 16 bytes into a destination of size 8
   4077 |         sprintf(label, "FREQ%d", nr + 1);
        |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

I think this is because before this patch it could work out, based on the
use of constants, that nr is never greater than 3, so the formatted
string will fit in the available space.

But now, although in practice that is still true, GCC can't see it

I wonder if the following is appropriate to add, either as squashed
into your patch as a separate patch. Arguably it is correct as
it allows provides enough space in the label buffer for all possible
values of nr, even though in practice only rather small values are used.

diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
index c723d6fe3d31..ce52b4080a19 100644
--- a/drivers/ptp/ptp_ocp.c
+++ b/drivers/ptp/ptp_ocp.c
@@ -4044,7 +4044,7 @@ _signal_summary_show(struct seq_file *s, struct ptp_ocp *bp, int nr)
 {
 	struct signal_reg __iomem *reg = bp->signal_out[nr]->mem;
 	struct ptp_ocp_signal *signal = &bp->signal[nr];
-	char label[8];
+	char label[16];
 	bool on;
 	u32 val;
 
@@ -4067,7 +4067,7 @@ static void
 _frequency_summary_show(struct seq_file *s, int nr,
 			struct frequency_reg __iomem *reg)
 {
-	char label[8];
+	char label[16];
 	bool on;
 	u32 val;