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

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

[Sagi Maimon]

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>
---
Addressed comments from Simon Horman:
 - https://www.spinics.net/lists/netdev/msg1089986.html
Changes since v1:
 - Increase label buffer size from 8 to 16 bytes to prevent potential buffer
   overflow warnings from GCC 14.2.0 during string formatting.
---
---
 drivers/ptp/ptp_ocp.c | 54 +++++++++++++++++++++++++++++++++++--------
 1 file changed, 45 insertions(+), 9 deletions(-)

diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
index 2ccdca4f6960..dd6de70de29b 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))
@@ -4008,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;
 
@@ -4031,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;
 
@@ -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 v2] ptp: ocp: Limit SMA/signal/freq counts in show/store functions

[Vadim Fedorenko]

On 08/05/2025 08:19, 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>
> ---
> Addressed comments from Simon Horman:
>   - https://www.spinics.net/lists/netdev/msg1089986.html
> Changes since v1:
>   - Increase label buffer size from 8 to 16 bytes to prevent potential buffer
>     overflow warnings from GCC 14.2.0 during string formatting.
> ---

Reviewed-by: Vadim Fedorenko <vadim.fedorenko@linux.dev>

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

[Simon Horman]

On Thu, May 08, 2025 at 10:19:01AM +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>
> ---
> Addressed comments from Simon Horman:
>  - https://www.spinics.net/lists/netdev/msg1089986.html
> Changes since v1:
>  - Increase label buffer size from 8 to 16 bytes to prevent potential buffer
>    overflow warnings from GCC 14.2.0 during string formatting.

Reviewed-by: Simon Horman <horms@kernel.org>

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

[Jakub Kicinski]

On Thu,  8 May 2025 10:19:01 +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

This line is too long. I think the recommended limit for commit message
is / was 72 or 74 chars.

> respect these limits, preventing out-of-bounds access and ensuring safe
> array handling.

What do you mean by out-of-bounds access here. Is there any access with
index > 4 possible? Or just with index > 1 for Adva?

We need more precise information about the problem to decide if this is
a fix or an improvement 

> +	bp->sma_nr  = 4;

nit: double space in all the sma_nr assignments

>  
>  	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;

Why are you returning 0 and not an error?

As a matter of fact why register the sysfs files for things which don't
exists?
-- 
pw-bot: cr

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

[Sagi Maimon]

On Sat, May 10, 2025 at 1:01 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Thu,  8 May 2025 10:19:01 +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
>
> This line is too long. I think the recommended limit for commit message
> is / was 72 or 74 chars.
>
will be fixed on next patch
> > respect these limits, preventing out-of-bounds access and ensuring safe
> > array handling.
>
> What do you mean by out-of-bounds access here. Is there any access with
> index > 4 possible? Or just with index > 1 for Adva?
>
index > 4 is possible via the sysfs commands, so this fix is general
for all boards
> We need more precise information about the problem to decide if this is
> a fix or an improvement
>
> > +     bp->sma_nr  = 4;
>
> nit: double space in all the sma_nr assignments
>
will be fixed on next patch
> >
> >       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;
>
> Why are you returning 0 and not an error?
>
will be fixed next patch
> As a matter of fact why register the sysfs files for things which don't
> exists?
> --
The number of SMAs initialized via sysfs is shared across all boards,
necessitating a modification to this mechanism. Additionally, only the
freq_in[] and signal_out[] arrays are causing NULL pointer
dereferences. To address these issues, I will submit two separate
patches: one to handle the NULL pointer dereferences in signals and
freq_in, and another to refactor the SMA initialization process.

> pw-bot: cr

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

[Sagi Maimon]

On Sun, May 11, 2025 at 11:16 AM Sagi Maimon <maimon.sagi@gmail.com> wrote:
>
> On Sat, May 10, 2025 at 1:01 AM Jakub Kicinski <kuba@kernel.org> wrote:
> >
> > On Thu,  8 May 2025 10:19:01 +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
> >
> > This line is too long. I think the recommended limit for commit message
> > is / was 72 or 74 chars.
> >
> will be fixed on next patch
> > > respect these limits, preventing out-of-bounds access and ensuring safe
> > > array handling.
> >
> > What do you mean by out-of-bounds access here. Is there any access with
> > index > 4 possible? Or just with index > 1 for Adva?
> >
> index > 4 is possible via the sysfs commands, so this fix is general
> for all boards
this is true for signals_nr and freq_in_nr arrays.
> > We need more precise information about the problem to decide if this is
> > a fix or an improvement
> >
> > > +     bp->sma_nr  = 4;
> >
> > nit: double space in all the sma_nr assignments
> >
> will be fixed on next patch
> > >
> > >       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;
> >
> > Why are you returning 0 and not an error?
> >
> will be fixed next patch
> > As a matter of fact why register the sysfs files for things which don't
> > exists?
> > --
> The number of SMAs initialized via sysfs is shared across all boards,
> necessitating a modification to this mechanism. Additionally, only the
> freq_in[] and signal_out[] arrays are causing NULL pointer
> dereferences. To address these issues, I will submit two separate
> patches: one to handle the NULL pointer dereferences in signals and
> freq_in, and another to refactor the SMA initialization process.
>
> > pw-bot: cr

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

[Sagi Maimon]

On Sun, May 11, 2025 at 12:03 PM Sagi Maimon <maimon.sagi@gmail.com> wrote:
>
> On Sun, May 11, 2025 at 11:16 AM Sagi Maimon <maimon.sagi@gmail.com> wrote:
> >
> > On Sat, May 10, 2025 at 1:01 AM Jakub Kicinski <kuba@kernel.org> wrote:
> > >
> > > On Thu,  8 May 2025 10:19:01 +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
> > >
> > > This line is too long. I think the recommended limit for commit message
> > > is / was 72 or 74 chars.
> > >
> > will be fixed on next patch
> > > > respect these limits, preventing out-of-bounds access and ensuring safe
> > > > array handling.
> > >
> > > What do you mean by out-of-bounds access here. Is there any access with
> > > index > 4 possible? Or just with index > 1 for Adva?
> > >
The sysfs interface restricts indices to a maximum of 4; however,
since an array of 4 signals/frequencies is always created and fully
accessible via sysfs—regardless of the actual number initialized—this
bug impacts any board that initializes fewer than 4
signals/frequencies.
> > > We need more precise information about the problem to decide if this is
> > > a fix or an improvement
> > >
> > > > +     bp->sma_nr  = 4;
> > >
> > > nit: double space in all the sma_nr assignments
> > >
> > will be fixed on next patch
> > > >
> > > >       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;
> > >
> > > Why are you returning 0 and not an error?
> > >
> > will be fixed next patch
> > > As a matter of fact why register the sysfs files for things which don't
> > > exists?
> > > --
> > The number of SMAs initialized via sysfs is shared across all boards,
> > necessitating a modification to this mechanism. Additionally, only the
> > freq_in[] and signal_out[] arrays are causing NULL pointer
> > dereferences. To address these issues, I will submit two separate
> > patches: one to handle the NULL pointer dereferences in signals and
> > freq_in, and another to refactor the SMA initialization process.
> >
> > > pw-bot: cr

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

[Jakub Kicinski]

On Sun, 11 May 2025 17:39:08 +0300 Sagi Maimon wrote:
> > > > What do you mean by out-of-bounds access here. Is there any access with
> > > > index > 4 possible? Or just with index > 1 for Adva?
> 
> The sysfs interface restricts indices to a maximum of 4; however,
> since an array of 4 signals/frequencies is always created and fully
> accessible via sysfs—regardless of the actual number initialized—this
> bug impacts any board that initializes fewer than 4
> signals/frequencies.

Right, but the bug is that user may write to registers which don't
exist? Or something will crash? We need to give backporters more info
about the impact of this bug. Can this crash the kernel?

As for sysfs exposing 4 entries, I think it's controlled by what groups
of attributes are added. So I think were possible we should create
attribute groups with only 2 entries for Adva. Eg. copy
fb_timecard_groups[] with just the correct entries, and in
ptp_ocp_fb_board_init() add an if which selects the right array.

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

[Sagi Maimon]

On Tue, May 13, 2025 at 3:07 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Sun, 11 May 2025 17:39:08 +0300 Sagi Maimon wrote:
> > > > > What do you mean by out-of-bounds access here. Is there any access with
> > > > > index > 4 possible? Or just with index > 1 for Adva?
> >
> > The sysfs interface restricts indices to a maximum of 4; however,
> > since an array of 4 signals/frequencies is always created and fully
> > accessible via sysfs—regardless of the actual number initialized—this
> > bug impacts any board that initializes fewer than 4
> > signals/frequencies.
>
> Right, but the bug is that user may write to registers which don't
> exist? Or something will crash? We need to give backporters more info
> about the impact of this bug. Can this crash the kernel?
>
it can not crash the kernel, it avoks kernel Oops (page_fault_oops),
the kernel the kernel recovering by terminating the process.
It will be added to the commit note.
> As for sysfs exposing 4 entries, I think it's controlled by what groups
> of attributes are added. So I think were possible we should create
> attribute groups with only 2 entries for Adva. Eg. copy
> fb_timecard_groups[] with just the correct entries, and in
> ptp_ocp_fb_board_init() add an if which selects the right array.
you are right (look at Vadim's reply too)
Adva has adva_timecard_groups with the correct entries, so the fix is
relevant only for signal_summary_show
and _frequency_summary_show