drivers/ptp/ptp_ocp.c | 54 +++++++++++++++++++++++++++++++++++-------- 1 file changed, 45 insertions(+), 9 deletions(-)
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.0On 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
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
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
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
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.
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
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>
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>
© 2016 - 2025 Red Hat, Inc.