1. Patchew
  2. linux
  3. View series

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

Sagi Maimon posted 1 patch 7 months, 2 weeks ago
There is a newer version of this series
drivers/ptp/ptp_ocp.c | 50 +++++++++++++++++++++++++++++++++++++------
1 file changed, 43 insertions(+), 7 deletions(-)
[PATCH v1] ptp: ocp: Limit SMA/signal/freq counts in show/store functions
Posted by Sagi Maimon 7 months, 2 weeks ago 
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>
---
 drivers/ptp/ptp_ocp.c | 50 +++++++++++++++++++++++++++++++++++++------
 1 file changed, 43 insertions(+), 7 deletions(-)

diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
index 2ccdca4f6960..80481449dcd0 100644
--- a/drivers/ptp/ptp_ocp.c
+++ b/drivers/ptp/ptp_ocp.c
@@ -315,6 +315,8 @@ struct ptp_ocp_serial_port {
 #define OCP_BOARD_ID_LEN		13
 #define OCP_SERIAL_LEN			6
 #define OCP_SMA_NUM			4
+#define OCP_SIGNAL_NUM			4
+#define OCP_FREQ_NUM			4
 
 enum {
 	PORT_GNSS,
@@ -342,8 +344,8 @@ struct ptp_ocp {
 	struct dcf_master_reg	__iomem *dcf_out;
 	struct dcf_slave_reg	__iomem *dcf_in;
 	struct tod_reg		__iomem *nmea_out;
-	struct frequency_reg	__iomem *freq_in[4];
-	struct ptp_ocp_ext_src	*signal_out[4];
+	struct frequency_reg	__iomem *freq_in[OCP_FREQ_NUM];
+	struct ptp_ocp_ext_src	*signal_out[OCP_SIGNAL_NUM];
 	struct ptp_ocp_ext_src	*pps;
 	struct ptp_ocp_ext_src	*ts0;
 	struct ptp_ocp_ext_src	*ts1;
@@ -378,10 +380,13 @@ struct ptp_ocp {
 	u32			utc_tai_offset;
 	u32			ts_window_adjust;
 	u64			fw_cap;
-	struct ptp_ocp_signal	signal[4];
+	struct ptp_ocp_signal	signal[OCP_SIGNAL_NUM];
 	struct ptp_ocp_sma_connector sma[OCP_SMA_NUM];
 	const struct ocp_sma_op *sma_op;
 	struct dpll_device *dpll;
+	int signals_nr;
+	int freq_in_nr;
+	int sma_nr;
 };
 
 #define OCP_REQ_TIMESTAMP	BIT(0)
@@ -2697,6 +2702,9 @@ ptp_ocp_fb_board_init(struct ptp_ocp *bp, struct ocp_resource *r)
 	bp->eeprom_map = fb_eeprom_map;
 	bp->fw_version = ioread32(&bp->image->version);
 	bp->sma_op = &ocp_fb_sma_op;
+	bp->signals_nr = 4;
+	bp->freq_in_nr = 4;
+	bp->sma_nr  = 4;
 
 	ptp_ocp_fb_set_version(bp);
 
@@ -2862,6 +2870,9 @@ ptp_ocp_art_board_init(struct ptp_ocp *bp, struct ocp_resource *r)
 	bp->fw_version = ioread32(&bp->reg->version);
 	bp->fw_tag = 2;
 	bp->sma_op = &ocp_art_sma_op;
+	bp->signals_nr = 4;
+	bp->freq_in_nr = 4;
+	bp->sma_nr  = 4;
 
 	/* Enable MAC serial port during initialisation */
 	iowrite32(1, &bp->board_config->mro50_serial_activate);
@@ -2888,6 +2899,9 @@ ptp_ocp_adva_board_init(struct ptp_ocp *bp, struct ocp_resource *r)
 	bp->flash_start = 0xA00000;
 	bp->eeprom_map = fb_eeprom_map;
 	bp->sma_op = &ocp_adva_sma_op;
+	bp->signals_nr = 2;
+	bp->freq_in_nr = 2;
+	bp->sma_nr  = 2;
 
 	version = ioread32(&bp->image->version);
 	/* if lower 16 bits are empty, this is the fw loader. */
@@ -3002,6 +3016,9 @@ ptp_ocp_sma_show(struct ptp_ocp *bp, int sma_nr, char *buf,
 	const struct ocp_selector * const *tbl;
 	u32 val;
 
+	if (sma_nr > bp->sma_nr)
+		return 0;
+
 	tbl = bp->sma_op->tbl;
 	val = ptp_ocp_sma_get(bp, sma_nr) & SMA_SELECT_MASK;
 
@@ -3091,6 +3108,9 @@ ptp_ocp_sma_store(struct ptp_ocp *bp, const char *buf, int sma_nr)
 	enum ptp_ocp_sma_mode mode;
 	int val;
 
+	if (sma_nr > bp->sma_nr)
+		return 0;
+
 	mode = sma->mode;
 	val = sma_parse_inputs(bp->sma_op->tbl, buf, &mode);
 	if (val < 0)
@@ -3190,6 +3210,9 @@ signal_store(struct device *dev, struct device_attribute *attr,
 	if (!argv)
 		return -ENOMEM;
 
+	if (gen >= bp->signals_nr)
+		return 0;
+
 	err = -EINVAL;
 	s.duty = bp->signal[gen].duty;
 	s.phase = bp->signal[gen].phase;
@@ -3247,6 +3270,10 @@ signal_show(struct device *dev, struct device_attribute *attr, char *buf)
 	int i;
 
 	i = (uintptr_t)ea->var;
+
+	if (i >= bp->signals_nr)
+		return 0;
+
 	signal = &bp->signal[i];
 
 	count = sysfs_emit(buf, "%llu %d %llu %d", signal->period,
@@ -3359,6 +3386,9 @@ seconds_store(struct device *dev, struct device_attribute *attr,
 	u32 val;
 	int err;
 
+	if (idx >= bp->freq_in_nr)
+		return 0;
+
 	err = kstrtou32(buf, 0, &val);
 	if (err)
 		return err;
@@ -3381,6 +3411,9 @@ seconds_show(struct device *dev, struct device_attribute *attr, char *buf)
 	int idx = (uintptr_t)ea->var;
 	u32 val;
 
+	if (idx >= bp->freq_in_nr)
+		return 0;
+
 	val = ioread32(&bp->freq_in[idx]->ctrl);
 	if (val & 1)
 		val = (val >> 8) & 0xff;
@@ -3402,6 +3435,9 @@ frequency_show(struct device *dev, struct device_attribute *attr, char *buf)
 	int idx = (uintptr_t)ea->var;
 	u32 val;
 
+	if (idx >= bp->freq_in_nr)
+		return 0;
+
 	val = ioread32(&bp->freq_in[idx]->status);
 	if (val & FREQ_STATUS_ERROR)
 		return sysfs_emit(buf, "error\n");
@@ -3975,7 +4011,7 @@ gpio_input_map(char *buf, struct ptp_ocp *bp, u16 map[][2], u16 bit,
 {
 	int i;
 
-	for (i = 0; i < 4; i++) {
+	for (i = 0; i < bp->sma_nr; i++) {
 		if (bp->sma[i].mode != SMA_MODE_IN)
 			continue;
 		if (map[i][0] & (1 << bit)) {
@@ -3995,7 +4031,7 @@ gpio_output_map(char *buf, struct ptp_ocp *bp, u16 map[][2], u16 bit)
 	int i;
 
 	strcpy(ans, "----");
-	for (i = 0; i < 4; i++) {
+	for (i = 0; i < bp->sma_nr; i++) {
 		if (bp->sma[i].mode != SMA_MODE_OUT)
 			continue;
 		if (map[i][1] & (1 << bit))
@@ -4175,11 +4211,11 @@ ptp_ocp_summary_show(struct seq_file *s, void *data)
 	}
 
 	if (bp->fw_cap & OCP_CAP_SIGNAL)
-		for (i = 0; i < 4; i++)
+		for (i = 0; i < bp->signals_nr; i++)
 			_signal_summary_show(s, bp, i);
 
 	if (bp->fw_cap & OCP_CAP_FREQ)
-		for (i = 0; i < 4; i++)
+		for (i = 0; i < bp->freq_in_nr; i++)
 			_frequency_summary_show(s, i, bp->freq_in[i]);
 
 	if (bp->irig_out) {
-- 
2.47.0
Re: [PATCH v1] ptp: ocp: Limit SMA/signal/freq counts in show/store functions
Posted by kernel test robot 7 months, 1 week ago 
Hi Sagi,

kernel test robot noticed the following build warnings:

[auto build test WARNING on net-next/main]
[also build test WARNING on net/main linus/master v6.15-rc5 next-20250507]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Sagi-Maimon/ptp-ocp-Limit-SMA-signal-freq-counts-in-show-store-functions/20250506-161305
base:   net-next/main
patch link:    https://lore.kernel.org/r/20250506080647.116702-1-maimon.sagi%40gmail.com
patch subject: [PATCH v1] ptp: ocp: Limit SMA/signal/freq counts in show/store functions
config: parisc-allmodconfig (https://download.01.org/0day-ci/archive/20250508/202505080859.Ke4zJAh1-lkp@intel.com/config)
compiler: hppa-linux-gcc (GCC) 14.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250508/202505080859.Ke4zJAh1-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202505080859.Ke4zJAh1-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/ptp/ptp_ocp.c: In function 'ptp_ocp_summary_show':
>> drivers/ptp/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:
   drivers/ptp/ptp_ocp.c:4052:24: note: directive argument in the range [-2147483639, 2147483647]
    4052 |         sprintf(label, "GEN%d", nr + 1);
         |                        ^~~~~~~
   drivers/ptp/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);
         |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/ptp/ptp_ocp.c: In function 'ptp_ocp_summary_show':
   drivers/ptp/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:
   drivers/ptp/ptp_ocp.c:4077:24: note: directive argument in the range [-2147483640, 2147483647]
    4077 |         sprintf(label, "FREQ%d", nr + 1);
         |                        ^~~~~~~~
   drivers/ptp/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);
         |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~


vim +4052 drivers/ptp/ptp_ocp.c

f67bf662d2cffa2 Jonathan Lemon 2021-09-14  4041  
b325af3cfab970e Jonathan Lemon 2022-03-10  4042  static void
b325af3cfab970e Jonathan Lemon 2022-03-10  4043  _signal_summary_show(struct seq_file *s, struct ptp_ocp *bp, int nr)
b325af3cfab970e Jonathan Lemon 2022-03-10  4044  {
b325af3cfab970e Jonathan Lemon 2022-03-10  4045  	struct signal_reg __iomem *reg = bp->signal_out[nr]->mem;
b325af3cfab970e Jonathan Lemon 2022-03-10  4046  	struct ptp_ocp_signal *signal = &bp->signal[nr];
b325af3cfab970e Jonathan Lemon 2022-03-10  4047  	char label[8];
b325af3cfab970e Jonathan Lemon 2022-03-10  4048  	bool on;
b325af3cfab970e Jonathan Lemon 2022-03-10  4049  	u32 val;
b325af3cfab970e Jonathan Lemon 2022-03-10  4050  
b325af3cfab970e Jonathan Lemon 2022-03-10  4051  	on = signal->running;
05fc65f3f5e45e8 Jonathan Lemon 2022-03-15 @4052  	sprintf(label, "GEN%d", nr + 1);
b325af3cfab970e Jonathan Lemon 2022-03-10  4053  	seq_printf(s, "%7s: %s, period:%llu duty:%d%% phase:%llu pol:%d",
b325af3cfab970e Jonathan Lemon 2022-03-10  4054  		   label, on ? " ON" : "OFF",
b325af3cfab970e Jonathan Lemon 2022-03-10  4055  		   signal->period, signal->duty, signal->phase,
b325af3cfab970e Jonathan Lemon 2022-03-10  4056  		   signal->polarity);
b325af3cfab970e Jonathan Lemon 2022-03-10  4057  
b325af3cfab970e Jonathan Lemon 2022-03-10  4058  	val = ioread32(&reg->enable);
b325af3cfab970e Jonathan Lemon 2022-03-10  4059  	seq_printf(s, " [%x", val);
b325af3cfab970e Jonathan Lemon 2022-03-10  4060  	val = ioread32(&reg->status);
b325af3cfab970e Jonathan Lemon 2022-03-10  4061  	seq_printf(s, " %x]", val);
b325af3cfab970e Jonathan Lemon 2022-03-10  4062  
b325af3cfab970e Jonathan Lemon 2022-03-10  4063  	seq_printf(s, " start:%llu\n", signal->start);
b325af3cfab970e Jonathan Lemon 2022-03-10  4064  }
b325af3cfab970e Jonathan Lemon 2022-03-10  4065  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Re: [PATCH v1] ptp: ocp: Limit SMA/signal/freq counts in show/store functions
Posted by Simon Horman 7 months, 1 week ago 
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;
Re: [PATCH v1] ptp: ocp: Limit SMA/signal/freq counts in show/store functions
Posted by Simon Horman 7 months, 1 week ago 
On Wed, May 07, 2025 at 08:46:30PM +0100, Simon Horman wrote:
> 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:

Sorry, I forgot to mention that this is an allmodconfig W=1 build
on x86_64.

...

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.