[PATCH v1] ptp: ocp: fix NULL deref in _signal_summary_show

[PATCH v1] ptp: ocp: fix NULL deref in _signal_summary_show

[Sagi Maimon]

Sysfs signal show operations can invoke _signal_summary_show before
signal_out array elements are initialized, causing a NULL pointer
dereference. Add NULL checks for signal_out elements to prevent kernel
crashes.

Fixes: b325af3cfab9 ("ptp: ocp: Add signal generators and update sysfs nodes")
Signed-off-by: Sagi Maimon <maimon.sagi@gmail.com>
---
 drivers/ptp/ptp_ocp.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
index 7945c6be1f7c..4c7893539cec 100644
--- a/drivers/ptp/ptp_ocp.c
+++ b/drivers/ptp/ptp_ocp.c
@@ -3963,6 +3963,9 @@ _signal_summary_show(struct seq_file *s, struct ptp_ocp *bp, int nr)
 	bool on;
 	u32 val;
 
+	if (!bp->signal_out[nr])
+		return;
+
 	on = signal->running;
 	sprintf(label, "GEN%d", nr + 1);
 	seq_printf(s, "%7s: %s, period:%llu duty:%d%% phase:%llu pol:%d",
-- 
2.47.0

Re: [PATCH v1] ptp: ocp: fix NULL deref in _signal_summary_show

[Vadim Fedorenko]

On 14/04/2025 09:54, Sagi Maimon wrote:
> Sysfs signal show operations can invoke _signal_summary_show before
> signal_out array elements are initialized, causing a NULL pointer
> dereference. Add NULL checks for signal_out elements to prevent kernel
> crashes.
> 
> Fixes: b325af3cfab9 ("ptp: ocp: Add signal generators and update sysfs nodes")
> Signed-off-by: Sagi Maimon <maimon.sagi@gmail.com>
> ---
>   drivers/ptp/ptp_ocp.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
> index 7945c6be1f7c..4c7893539cec 100644
> --- a/drivers/ptp/ptp_ocp.c
> +++ b/drivers/ptp/ptp_ocp.c
> @@ -3963,6 +3963,9 @@ _signal_summary_show(struct seq_file *s, struct ptp_ocp *bp, int nr)
>   	bool on;
>   	u32 val;
>   
> +	if (!bp->signal_out[nr])
> +		return;
> +
>   	on = signal->running;
>   	sprintf(label, "GEN%d", nr + 1);
>   	seq_printf(s, "%7s: %s, period:%llu duty:%d%% phase:%llu pol:%d",

That's not correct, the dereference of bp->signal_out[nr] happens before
the check. But I just wonder how can that even happen?

I believe the proper fix is to move ptp_ocp_attr_group_add() closer to
the end of ptp_ocp_adva_board_init() like it's done for other boards.

--
pw-bot: cr

Re: [PATCH v1] ptp: ocp: fix NULL deref in _signal_summary_show

[Sagi Maimon]

On Mon, Apr 14, 2025 at 12:37 PM Vadim Fedorenko
<vadim.fedorenko@linux.dev> wrote:
>
> On 14/04/2025 09:54, Sagi Maimon wrote:
> > Sysfs signal show operations can invoke _signal_summary_show before
> > signal_out array elements are initialized, causing a NULL pointer
> > dereference. Add NULL checks for signal_out elements to prevent kernel
> > crashes.
> >
> > Fixes: b325af3cfab9 ("ptp: ocp: Add signal generators and update sysfs nodes")
> > Signed-off-by: Sagi Maimon <maimon.sagi@gmail.com>
> > ---
> >   drivers/ptp/ptp_ocp.c | 3 +++
> >   1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
> > index 7945c6be1f7c..4c7893539cec 100644
> > --- a/drivers/ptp/ptp_ocp.c
> > +++ b/drivers/ptp/ptp_ocp.c
> > @@ -3963,6 +3963,9 @@ _signal_summary_show(struct seq_file *s, struct ptp_ocp *bp, int nr)
> >       bool on;
> >       u32 val;
> >
> > +     if (!bp->signal_out[nr])
> > +             return;
> > +
> >       on = signal->running;
> >       sprintf(label, "GEN%d", nr + 1);
> >       seq_printf(s, "%7s: %s, period:%llu duty:%d%% phase:%llu pol:%d",
>
> That's not correct, the dereference of bp->signal_out[nr] happens before
> the check. But I just wonder how can that even happen?
>
The scenario (our case): on ptp_ocp_adva_board_init we
initiate only signals 0 and 1 so 2 and 3 are NULL.
Later ptp_ocp_summary_show runs on all 4 signals and calls _signal_summary_show
when calling signal 2 or 3  the dereference occurs.
can you please explain: " the dereference of bp->signal_out[nr] happens before
the check", where exactly? do you mean in those lines:
struct signal_reg __iomem *reg = bp->signal_out[nr]->mem;
struct ptp_ocp_signal *signal = &bp->signal[nr];
> I believe the proper fix is to move ptp_ocp_attr_group_add() closer to
> the end of ptp_ocp_adva_board_init() like it's done for other boards.
>
> --
> pw-bot: cr

Re: [PATCH v1] ptp: ocp: fix NULL deref in _signal_summary_show

[Vadim Fedorenko]

On 14/04/2025 11:56, Sagi Maimon wrote:
> On Mon, Apr 14, 2025 at 12:37 PM Vadim Fedorenko
> <vadim.fedorenko@linux.dev> wrote:
>>
>> On 14/04/2025 09:54, Sagi Maimon wrote:
>>> Sysfs signal show operations can invoke _signal_summary_show before
>>> signal_out array elements are initialized, causing a NULL pointer
>>> dereference. Add NULL checks for signal_out elements to prevent kernel
>>> crashes.
>>>
>>> Fixes: b325af3cfab9 ("ptp: ocp: Add signal generators and update sysfs nodes")
>>> Signed-off-by: Sagi Maimon <maimon.sagi@gmail.com>
>>> ---
>>>    drivers/ptp/ptp_ocp.c | 3 +++
>>>    1 file changed, 3 insertions(+)
>>>
>>> diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
>>> index 7945c6be1f7c..4c7893539cec 100644
>>> --- a/drivers/ptp/ptp_ocp.c
>>> +++ b/drivers/ptp/ptp_ocp.c
>>> @@ -3963,6 +3963,9 @@ _signal_summary_show(struct seq_file *s, struct ptp_ocp *bp, int nr)
>>>        bool on;
>>>        u32 val;
>>>
>>> +     if (!bp->signal_out[nr])
>>> +             return;
>>> +
>>>        on = signal->running;
>>>        sprintf(label, "GEN%d", nr + 1);
>>>        seq_printf(s, "%7s: %s, period:%llu duty:%d%% phase:%llu pol:%d",
>>
>> That's not correct, the dereference of bp->signal_out[nr] happens before
>> the check. But I just wonder how can that even happen?
>>
> The scenario (our case): on ptp_ocp_adva_board_init we
> initiate only signals 0 and 1 so 2 and 3 are NULL.
> Later ptp_ocp_summary_show runs on all 4 signals and calls _signal_summary_show
> when calling signal 2 or 3  the dereference occurs.
> can you please explain: " the dereference of bp->signal_out[nr] happens before
> the check", where exactly? do you mean in those lines:
> struct signal_reg __iomem *reg = bp->signal_out[nr]->mem;
    ^^^
yes, this is the line which dereferences the pointer.

but in case you have only 2 pins to configure, why the driver exposes 4
SMAs? You can simply adjust the attributes (adva_timecard_attrs).

> struct ptp_ocp_signal *signal = &bp->signal[nr];
>> I believe the proper fix is to move ptp_ocp_attr_group_add() closer to
>> the end of ptp_ocp_adva_board_init() like it's done for other boards.
>>
>> --
>> pw-bot: cr

Re: [PATCH v1] ptp: ocp: fix NULL deref in _signal_summary_show

[Sagi Maimon]

On Mon, Apr 14, 2025 at 2:09 PM Vadim Fedorenko
<vadim.fedorenko@linux.dev> wrote:
>
> On 14/04/2025 11:56, Sagi Maimon wrote:
> > On Mon, Apr 14, 2025 at 12:37 PM Vadim Fedorenko
> > <vadim.fedorenko@linux.dev> wrote:
> >>
> >> On 14/04/2025 09:54, Sagi Maimon wrote:
> >>> Sysfs signal show operations can invoke _signal_summary_show before
> >>> signal_out array elements are initialized, causing a NULL pointer
> >>> dereference. Add NULL checks for signal_out elements to prevent kernel
> >>> crashes.
> >>>
> >>> Fixes: b325af3cfab9 ("ptp: ocp: Add signal generators and update sysfs nodes")
> >>> Signed-off-by: Sagi Maimon <maimon.sagi@gmail.com>
> >>> ---
> >>>    drivers/ptp/ptp_ocp.c | 3 +++
> >>>    1 file changed, 3 insertions(+)
> >>>
> >>> diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
> >>> index 7945c6be1f7c..4c7893539cec 100644
> >>> --- a/drivers/ptp/ptp_ocp.c
> >>> +++ b/drivers/ptp/ptp_ocp.c
> >>> @@ -3963,6 +3963,9 @@ _signal_summary_show(struct seq_file *s, struct ptp_ocp *bp, int nr)
> >>>        bool on;
> >>>        u32 val;
> >>>
> >>> +     if (!bp->signal_out[nr])
> >>> +             return;
> >>> +
> >>>        on = signal->running;
> >>>        sprintf(label, "GEN%d", nr + 1);
> >>>        seq_printf(s, "%7s: %s, period:%llu duty:%d%% phase:%llu pol:%d",
> >>
> >> That's not correct, the dereference of bp->signal_out[nr] happens before
> >> the check. But I just wonder how can that even happen?
> >>
> > The scenario (our case): on ptp_ocp_adva_board_init we
> > initiate only signals 0 and 1 so 2 and 3 are NULL.
> > Later ptp_ocp_summary_show runs on all 4 signals and calls _signal_summary_show
> > when calling signal 2 or 3  the dereference occurs.
> > can you please explain: " the dereference of bp->signal_out[nr] happens before
> > the check", where exactly? do you mean in those lines:
> > struct signal_reg __iomem *reg = bp->signal_out[nr]->mem;
>     ^^^
> yes, this is the line which dereferences the pointer.
>
> but in case you have only 2 pins to configure, why the driver exposes 4
> SMAs? You can simply adjust the attributes (adva_timecard_attrs).
>
I can (and will) expose only 2 sma in adva_timecard_attrs, but still
ptp_ocp_summary_show runs
on all 4 signals and not only on the on that exposed, is it not a bug?
> > struct ptp_ocp_signal *signal = &bp->signal[nr];
> >> I believe the proper fix is to move ptp_ocp_attr_group_add() closer to
> >> the end of ptp_ocp_adva_board_init() like it's done for other boards.
> >>
> >> --
> >> pw-bot: cr
>

Re: [PATCH v1] ptp: ocp: fix NULL deref in _signal_summary_show

[Vadim Fedorenko]

On 14/04/2025 12:38, Sagi Maimon wrote:
> On Mon, Apr 14, 2025 at 2:09 PM Vadim Fedorenko
> <vadim.fedorenko@linux.dev> wrote:
>>
>> On 14/04/2025 11:56, Sagi Maimon wrote:
>>> On Mon, Apr 14, 2025 at 12:37 PM Vadim Fedorenko
>>> <vadim.fedorenko@linux.dev> wrote:
>>>>
>>>> On 14/04/2025 09:54, Sagi Maimon wrote:
>>>>> Sysfs signal show operations can invoke _signal_summary_show before
>>>>> signal_out array elements are initialized, causing a NULL pointer
>>>>> dereference. Add NULL checks for signal_out elements to prevent kernel
>>>>> crashes.
>>>>>
>>>>> Fixes: b325af3cfab9 ("ptp: ocp: Add signal generators and update sysfs nodes")
>>>>> Signed-off-by: Sagi Maimon <maimon.sagi@gmail.com>
>>>>> ---
>>>>>     drivers/ptp/ptp_ocp.c | 3 +++
>>>>>     1 file changed, 3 insertions(+)
>>>>>
>>>>> diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
>>>>> index 7945c6be1f7c..4c7893539cec 100644
>>>>> --- a/drivers/ptp/ptp_ocp.c
>>>>> +++ b/drivers/ptp/ptp_ocp.c
>>>>> @@ -3963,6 +3963,9 @@ _signal_summary_show(struct seq_file *s, struct ptp_ocp *bp, int nr)
>>>>>         bool on;
>>>>>         u32 val;
>>>>>
>>>>> +     if (!bp->signal_out[nr])
>>>>> +             return;
>>>>> +
>>>>>         on = signal->running;
>>>>>         sprintf(label, "GEN%d", nr + 1);
>>>>>         seq_printf(s, "%7s: %s, period:%llu duty:%d%% phase:%llu pol:%d",
>>>>
>>>> That's not correct, the dereference of bp->signal_out[nr] happens before
>>>> the check. But I just wonder how can that even happen?
>>>>
>>> The scenario (our case): on ptp_ocp_adva_board_init we
>>> initiate only signals 0 and 1 so 2 and 3 are NULL.
>>> Later ptp_ocp_summary_show runs on all 4 signals and calls _signal_summary_show
>>> when calling signal 2 or 3  the dereference occurs.
>>> can you please explain: " the dereference of bp->signal_out[nr] happens before
>>> the check", where exactly? do you mean in those lines:
>>> struct signal_reg __iomem *reg = bp->signal_out[nr]->mem;
>>      ^^^
>> yes, this is the line which dereferences the pointer.
>>
>> but in case you have only 2 pins to configure, why the driver exposes 4
>> SMAs? You can simply adjust the attributes (adva_timecard_attrs).
>>
> I can (and will) expose only 2 sma in adva_timecard_attrs, but still
> ptp_ocp_summary_show runs
> on all 4 signals and not only on the on that exposed, is it not a bug?

Yeah, it's a bug, but different one, and we have to fix it other way.

>>> struct ptp_ocp_signal *signal = &bp->signal[nr];
>>>> I believe the proper fix is to move ptp_ocp_attr_group_add() closer to
>>>> the end of ptp_ocp_adva_board_init() like it's done for other boards.
>>>>
>>>> --
>>>> pw-bot: cr
>>

Re: [PATCH v1] ptp: ocp: fix NULL deref in _signal_summary_show

[Sagi Maimon]

On Mon, Apr 14, 2025 at 4:01 PM Vadim Fedorenko
<vadim.fedorenko@linux.dev> wrote:
>
> On 14/04/2025 12:38, Sagi Maimon wrote:
> > On Mon, Apr 14, 2025 at 2:09 PM Vadim Fedorenko
> > <vadim.fedorenko@linux.dev> wrote:
> >>
> >> On 14/04/2025 11:56, Sagi Maimon wrote:
> >>> On Mon, Apr 14, 2025 at 12:37 PM Vadim Fedorenko
> >>> <vadim.fedorenko@linux.dev> wrote:
> >>>>
> >>>> On 14/04/2025 09:54, Sagi Maimon wrote:
> >>>>> Sysfs signal show operations can invoke _signal_summary_show before
> >>>>> signal_out array elements are initialized, causing a NULL pointer
> >>>>> dereference. Add NULL checks for signal_out elements to prevent kernel
> >>>>> crashes.
> >>>>>
> >>>>> Fixes: b325af3cfab9 ("ptp: ocp: Add signal generators and update sysfs nodes")
> >>>>> Signed-off-by: Sagi Maimon <maimon.sagi@gmail.com>
> >>>>> ---
> >>>>>     drivers/ptp/ptp_ocp.c | 3 +++
> >>>>>     1 file changed, 3 insertions(+)
> >>>>>
> >>>>> diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
> >>>>> index 7945c6be1f7c..4c7893539cec 100644
> >>>>> --- a/drivers/ptp/ptp_ocp.c
> >>>>> +++ b/drivers/ptp/ptp_ocp.c
> >>>>> @@ -3963,6 +3963,9 @@ _signal_summary_show(struct seq_file *s, struct ptp_ocp *bp, int nr)
> >>>>>         bool on;
> >>>>>         u32 val;
> >>>>>
> >>>>> +     if (!bp->signal_out[nr])
> >>>>> +             return;
> >>>>> +
> >>>>>         on = signal->running;
> >>>>>         sprintf(label, "GEN%d", nr + 1);
> >>>>>         seq_printf(s, "%7s: %s, period:%llu duty:%d%% phase:%llu pol:%d",
> >>>>
> >>>> That's not correct, the dereference of bp->signal_out[nr] happens before
> >>>> the check. But I just wonder how can that even happen?
> >>>>
> >>> The scenario (our case): on ptp_ocp_adva_board_init we
> >>> initiate only signals 0 and 1 so 2 and 3 are NULL.
> >>> Later ptp_ocp_summary_show runs on all 4 signals and calls _signal_summary_show
> >>> when calling signal 2 or 3  the dereference occurs.
> >>> can you please explain: " the dereference of bp->signal_out[nr] happens before
> >>> the check", where exactly? do you mean in those lines:
> >>> struct signal_reg __iomem *reg = bp->signal_out[nr]->mem;
> >>      ^^^
> >> yes, this is the line which dereferences the pointer.
> >>
> >> but in case you have only 2 pins to configure, why the driver exposes 4
> >> SMAs? You can simply adjust the attributes (adva_timecard_attrs).
> >>
> > I can (and will) expose only 2 sma in adva_timecard_attrs, but still
> > ptp_ocp_summary_show runs
> > on all 4 signals and not only on the on that exposed, is it not a bug?
>
> Yeah, it's a bug, but different one, and we have to fix it other way.
>
Do you want to instruct me how to fix it , or will you fix it?
> >>> struct ptp_ocp_signal *signal = &bp->signal[nr];
> >>>> I believe the proper fix is to move ptp_ocp_attr_group_add() closer to
> >>>> the end of ptp_ocp_adva_board_init() like it's done for other boards.
> >>>>
> >>>> --
> >>>> pw-bot: cr
> >>
>

Re: [PATCH v1] ptp: ocp: fix NULL deref in _signal_summary_show

[Vadim Fedorenko]

On 14/04/2025 14:43, Sagi Maimon wrote:
> On Mon, Apr 14, 2025 at 4:01 PM Vadim Fedorenko
> <vadim.fedorenko@linux.dev> wrote:
>>
>> On 14/04/2025 12:38, Sagi Maimon wrote:
>>> On Mon, Apr 14, 2025 at 2:09 PM Vadim Fedorenko
>>> <vadim.fedorenko@linux.dev> wrote:
>>>>
>>>> On 14/04/2025 11:56, Sagi Maimon wrote:
>>>>> On Mon, Apr 14, 2025 at 12:37 PM Vadim Fedorenko
>>>>> <vadim.fedorenko@linux.dev> wrote:
>>>>>>
>>>>>> On 14/04/2025 09:54, Sagi Maimon wrote:
>>>>>>> Sysfs signal show operations can invoke _signal_summary_show before
>>>>>>> signal_out array elements are initialized, causing a NULL pointer
>>>>>>> dereference. Add NULL checks for signal_out elements to prevent kernel
>>>>>>> crashes.
>>>>>>>
>>>>>>> Fixes: b325af3cfab9 ("ptp: ocp: Add signal generators and update sysfs nodes")
>>>>>>> Signed-off-by: Sagi Maimon <maimon.sagi@gmail.com>
>>>>>>> ---
>>>>>>>      drivers/ptp/ptp_ocp.c | 3 +++
>>>>>>>      1 file changed, 3 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
>>>>>>> index 7945c6be1f7c..4c7893539cec 100644
>>>>>>> --- a/drivers/ptp/ptp_ocp.c
>>>>>>> +++ b/drivers/ptp/ptp_ocp.c
>>>>>>> @@ -3963,6 +3963,9 @@ _signal_summary_show(struct seq_file *s, struct ptp_ocp *bp, int nr)
>>>>>>>          bool on;
>>>>>>>          u32 val;
>>>>>>>
>>>>>>> +     if (!bp->signal_out[nr])
>>>>>>> +             return;
>>>>>>> +
>>>>>>>          on = signal->running;
>>>>>>>          sprintf(label, "GEN%d", nr + 1);
>>>>>>>          seq_printf(s, "%7s: %s, period:%llu duty:%d%% phase:%llu pol:%d",
>>>>>>
>>>>>> That's not correct, the dereference of bp->signal_out[nr] happens before
>>>>>> the check. But I just wonder how can that even happen?
>>>>>>
>>>>> The scenario (our case): on ptp_ocp_adva_board_init we
>>>>> initiate only signals 0 and 1 so 2 and 3 are NULL.
>>>>> Later ptp_ocp_summary_show runs on all 4 signals and calls _signal_summary_show
>>>>> when calling signal 2 or 3  the dereference occurs.
>>>>> can you please explain: " the dereference of bp->signal_out[nr] happens before
>>>>> the check", where exactly? do you mean in those lines:
>>>>> struct signal_reg __iomem *reg = bp->signal_out[nr]->mem;
>>>>       ^^^
>>>> yes, this is the line which dereferences the pointer.
>>>>
>>>> but in case you have only 2 pins to configure, why the driver exposes 4
>>>> SMAs? You can simply adjust the attributes (adva_timecard_attrs).
>>>>
>>> I can (and will) expose only 2 sma in adva_timecard_attrs, but still
>>> ptp_ocp_summary_show runs
>>> on all 4 signals and not only on the on that exposed, is it not a bug?
>>
>> Yeah, it's a bug, but different one, and we have to fix it other way.
>>
> Do you want to instruct me how to fix it , or will you fix it?

well, the original device structure was not designed to have the amount
of SMAs less than 4. We have to introduce another field to store actual
amount of SMAs to work with, and adjust the code to check the value. The
best solution would be to keep maximum amount of 4 SMAs in the structure
but create a helper which will init new field and will have
BUILD_BUG_ON() to prevent having more SMAs than fixed size array for
them. That will solve your problem, but I will need to check it on the
HW we run.

>>>>> struct ptp_ocp_signal *signal = &bp->signal[nr];
>>>>>> I believe the proper fix is to move ptp_ocp_attr_group_add() closer to
>>>>>> the end of ptp_ocp_adva_board_init() like it's done for other boards.
>>>>>>
>>>>>> --
>>>>>> pw-bot: cr
>>>>
>>

Re: [PATCH v1] ptp: ocp: fix NULL deref in _signal_summary_show

[Sagi Maimon]

On Mon, Apr 14, 2025 at 4:55 PM Vadim Fedorenko
<vadim.fedorenko@linux.dev> wrote:
>
> On 14/04/2025 14:43, Sagi Maimon wrote:
> > On Mon, Apr 14, 2025 at 4:01 PM Vadim Fedorenko
> > <vadim.fedorenko@linux.dev> wrote:
> >>
> >> On 14/04/2025 12:38, Sagi Maimon wrote:
> >>> On Mon, Apr 14, 2025 at 2:09 PM Vadim Fedorenko
> >>> <vadim.fedorenko@linux.dev> wrote:
> >>>>
> >>>> On 14/04/2025 11:56, Sagi Maimon wrote:
> >>>>> On Mon, Apr 14, 2025 at 12:37 PM Vadim Fedorenko
> >>>>> <vadim.fedorenko@linux.dev> wrote:
> >>>>>>
> >>>>>> On 14/04/2025 09:54, Sagi Maimon wrote:
> >>>>>>> Sysfs signal show operations can invoke _signal_summary_show before
> >>>>>>> signal_out array elements are initialized, causing a NULL pointer
> >>>>>>> dereference. Add NULL checks for signal_out elements to prevent kernel
> >>>>>>> crashes.
> >>>>>>>
> >>>>>>> Fixes: b325af3cfab9 ("ptp: ocp: Add signal generators and update sysfs nodes")
> >>>>>>> Signed-off-by: Sagi Maimon <maimon.sagi@gmail.com>
> >>>>>>> ---
> >>>>>>>      drivers/ptp/ptp_ocp.c | 3 +++
> >>>>>>>      1 file changed, 3 insertions(+)
> >>>>>>>
> >>>>>>> diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
> >>>>>>> index 7945c6be1f7c..4c7893539cec 100644
> >>>>>>> --- a/drivers/ptp/ptp_ocp.c
> >>>>>>> +++ b/drivers/ptp/ptp_ocp.c
> >>>>>>> @@ -3963,6 +3963,9 @@ _signal_summary_show(struct seq_file *s, struct ptp_ocp *bp, int nr)
> >>>>>>>          bool on;
> >>>>>>>          u32 val;
> >>>>>>>
> >>>>>>> +     if (!bp->signal_out[nr])
> >>>>>>> +             return;
> >>>>>>> +
> >>>>>>>          on = signal->running;
> >>>>>>>          sprintf(label, "GEN%d", nr + 1);
> >>>>>>>          seq_printf(s, "%7s: %s, period:%llu duty:%d%% phase:%llu pol:%d",
> >>>>>>
> >>>>>> That's not correct, the dereference of bp->signal_out[nr] happens before
> >>>>>> the check. But I just wonder how can that even happen?
> >>>>>>
> >>>>> The scenario (our case): on ptp_ocp_adva_board_init we
> >>>>> initiate only signals 0 and 1 so 2 and 3 are NULL.
> >>>>> Later ptp_ocp_summary_show runs on all 4 signals and calls _signal_summary_show
> >>>>> when calling signal 2 or 3  the dereference occurs.
> >>>>> can you please explain: " the dereference of bp->signal_out[nr] happens before
> >>>>> the check", where exactly? do you mean in those lines:
> >>>>> struct signal_reg __iomem *reg = bp->signal_out[nr]->mem;
> >>>>       ^^^
> >>>> yes, this is the line which dereferences the pointer.
> >>>>
> >>>> but in case you have only 2 pins to configure, why the driver exposes 4
> >>>> SMAs? You can simply adjust the attributes (adva_timecard_attrs).
> >>>>
> >>> I can (and will) expose only 2 sma in adva_timecard_attrs, but still
> >>> ptp_ocp_summary_show runs
> >>> on all 4 signals and not only on the on that exposed, is it not a bug?
> >>
> >> Yeah, it's a bug, but different one, and we have to fix it other way.
> >>
> > Do you want to instruct me how to fix it , or will you fix it?
>
> well, the original device structure was not designed to have the amount
> of SMAs less than 4. We have to introduce another field to store actual
> amount of SMAs to work with, and adjust the code to check the value. The
> best solution would be to keep maximum amount of 4 SMAs in the structure
> but create a helper which will init new field and will have
> BUILD_BUG_ON() to prevent having more SMAs than fixed size array for
> them. That will solve your problem, but I will need to check it on the
> HW we run.
>
just to be clear you will write the fix and test it on your HW, so you
don't want me to write the fix?
> >>>>> struct ptp_ocp_signal *signal = &bp->signal[nr];
> >>>>>> I believe the proper fix is to move ptp_ocp_attr_group_add() closer to
> >>>>>> the end of ptp_ocp_adva_board_init() like it's done for other boards.
> >>>>>>
> >>>>>> --
> >>>>>> pw-bot: cr
> >>>>
> >>
>

Re: [PATCH v1] ptp: ocp: fix NULL deref in _signal_summary_show

[Vadim Fedorenko]

On 16/04/2025 07:33, Sagi Maimon wrote:
> On Mon, Apr 14, 2025 at 4:55 PM Vadim Fedorenko
> <vadim.fedorenko@linux.dev> wrote:
>>
>> On 14/04/2025 14:43, Sagi Maimon wrote:
>>> On Mon, Apr 14, 2025 at 4:01 PM Vadim Fedorenko
>>> <vadim.fedorenko@linux.dev> wrote:
>>>>
>>>> On 14/04/2025 12:38, Sagi Maimon wrote:
>>>>> On Mon, Apr 14, 2025 at 2:09 PM Vadim Fedorenko
>>>>> <vadim.fedorenko@linux.dev> wrote:
>>>>>>
>>>>>> On 14/04/2025 11:56, Sagi Maimon wrote:
>>>>>>> On Mon, Apr 14, 2025 at 12:37 PM Vadim Fedorenko
>>>>>>> <vadim.fedorenko@linux.dev> wrote:
>>>>>>>>
>>>>>>>> On 14/04/2025 09:54, Sagi Maimon wrote:
>>>>>>>>> Sysfs signal show operations can invoke _signal_summary_show before
>>>>>>>>> signal_out array elements are initialized, causing a NULL pointer
>>>>>>>>> dereference. Add NULL checks for signal_out elements to prevent kernel
>>>>>>>>> crashes.
>>>>>>>>>
>>>>>>>>> Fixes: b325af3cfab9 ("ptp: ocp: Add signal generators and update sysfs nodes")
>>>>>>>>> Signed-off-by: Sagi Maimon <maimon.sagi@gmail.com>
>>>>>>>>> ---
>>>>>>>>>       drivers/ptp/ptp_ocp.c | 3 +++
>>>>>>>>>       1 file changed, 3 insertions(+)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
>>>>>>>>> index 7945c6be1f7c..4c7893539cec 100644
>>>>>>>>> --- a/drivers/ptp/ptp_ocp.c
>>>>>>>>> +++ b/drivers/ptp/ptp_ocp.c
>>>>>>>>> @@ -3963,6 +3963,9 @@ _signal_summary_show(struct seq_file *s, struct ptp_ocp *bp, int nr)
>>>>>>>>>           bool on;
>>>>>>>>>           u32 val;
>>>>>>>>>
>>>>>>>>> +     if (!bp->signal_out[nr])
>>>>>>>>> +             return;
>>>>>>>>> +
>>>>>>>>>           on = signal->running;
>>>>>>>>>           sprintf(label, "GEN%d", nr + 1);
>>>>>>>>>           seq_printf(s, "%7s: %s, period:%llu duty:%d%% phase:%llu pol:%d",
>>>>>>>>
>>>>>>>> That's not correct, the dereference of bp->signal_out[nr] happens before
>>>>>>>> the check. But I just wonder how can that even happen?
>>>>>>>>
>>>>>>> The scenario (our case): on ptp_ocp_adva_board_init we
>>>>>>> initiate only signals 0 and 1 so 2 and 3 are NULL.
>>>>>>> Later ptp_ocp_summary_show runs on all 4 signals and calls _signal_summary_show
>>>>>>> when calling signal 2 or 3  the dereference occurs.
>>>>>>> can you please explain: " the dereference of bp->signal_out[nr] happens before
>>>>>>> the check", where exactly? do you mean in those lines:
>>>>>>> struct signal_reg __iomem *reg = bp->signal_out[nr]->mem;
>>>>>>        ^^^
>>>>>> yes, this is the line which dereferences the pointer.
>>>>>>
>>>>>> but in case you have only 2 pins to configure, why the driver exposes 4
>>>>>> SMAs? You can simply adjust the attributes (adva_timecard_attrs).
>>>>>>
>>>>> I can (and will) expose only 2 sma in adva_timecard_attrs, but still
>>>>> ptp_ocp_summary_show runs
>>>>> on all 4 signals and not only on the on that exposed, is it not a bug?
>>>>
>>>> Yeah, it's a bug, but different one, and we have to fix it other way.
>>>>
>>> Do you want to instruct me how to fix it , or will you fix it?
>>
>> well, the original device structure was not designed to have the amount
>> of SMAs less than 4. We have to introduce another field to store actual
>> amount of SMAs to work with, and adjust the code to check the value. The
>> best solution would be to keep maximum amount of 4 SMAs in the structure
>> but create a helper which will init new field and will have
>> BUILD_BUG_ON() to prevent having more SMAs than fixed size array for
>> them. That will solve your problem, but I will need to check it on the
>> HW we run.
>>
> just to be clear you will write the fix and test it on your HW, so you
> don't want me to write the fix?

Well, it would be great if you can write the code which will make SMA
functions flexible to the amount of pin the HW has. All our HW has fixed
amount of 4 pins that's why the driver was coded with constants. Now
your hardware has slightly different amount of pins, so it needs
adjustments to the driver to work properly. I just want to be sure that
any adjustments will not break my HW - that's what I meant saying I'll
test it.

>>>>>>> struct ptp_ocp_signal *signal = &bp->signal[nr];
>>>>>>>> I believe the proper fix is to move ptp_ocp_attr_group_add() closer to
>>>>>>>> the end of ptp_ocp_adva_board_init() like it's done for other boards.
>>>>>>>>
>>>>>>>> --
>>>>>>>> pw-bot: cr
>>>>>>
>>>>
>>

Re: [PATCH v1] ptp: ocp: fix NULL deref in _signal_summary_show

[Sagi Maimon]

On Wed, Apr 16, 2025 at 1:35 PM Vadim Fedorenko
<vadim.fedorenko@linux.dev> wrote:
>
> On 16/04/2025 07:33, Sagi Maimon wrote:
> > On Mon, Apr 14, 2025 at 4:55 PM Vadim Fedorenko
> > <vadim.fedorenko@linux.dev> wrote:
> >>
> >> On 14/04/2025 14:43, Sagi Maimon wrote:
> >>> On Mon, Apr 14, 2025 at 4:01 PM Vadim Fedorenko
> >>> <vadim.fedorenko@linux.dev> wrote:
> >>>>
> >>>> On 14/04/2025 12:38, Sagi Maimon wrote:
> >>>>> On Mon, Apr 14, 2025 at 2:09 PM Vadim Fedorenko
> >>>>> <vadim.fedorenko@linux.dev> wrote:
> >>>>>>
> >>>>>> On 14/04/2025 11:56, Sagi Maimon wrote:
> >>>>>>> On Mon, Apr 14, 2025 at 12:37 PM Vadim Fedorenko
> >>>>>>> <vadim.fedorenko@linux.dev> wrote:
> >>>>>>>>
> >>>>>>>> On 14/04/2025 09:54, Sagi Maimon wrote:
> >>>>>>>>> Sysfs signal show operations can invoke _signal_summary_show before
> >>>>>>>>> signal_out array elements are initialized, causing a NULL pointer
> >>>>>>>>> dereference. Add NULL checks for signal_out elements to prevent kernel
> >>>>>>>>> crashes.
> >>>>>>>>>
> >>>>>>>>> Fixes: b325af3cfab9 ("ptp: ocp: Add signal generators and update sysfs nodes")
> >>>>>>>>> Signed-off-by: Sagi Maimon <maimon.sagi@gmail.com>
> >>>>>>>>> ---
> >>>>>>>>>       drivers/ptp/ptp_ocp.c | 3 +++
> >>>>>>>>>       1 file changed, 3 insertions(+)
> >>>>>>>>>
> >>>>>>>>> diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
> >>>>>>>>> index 7945c6be1f7c..4c7893539cec 100644
> >>>>>>>>> --- a/drivers/ptp/ptp_ocp.c
> >>>>>>>>> +++ b/drivers/ptp/ptp_ocp.c
> >>>>>>>>> @@ -3963,6 +3963,9 @@ _signal_summary_show(struct seq_file *s, struct ptp_ocp *bp, int nr)
> >>>>>>>>>           bool on;
> >>>>>>>>>           u32 val;
> >>>>>>>>>
> >>>>>>>>> +     if (!bp->signal_out[nr])
> >>>>>>>>> +             return;
> >>>>>>>>> +
> >>>>>>>>>           on = signal->running;
> >>>>>>>>>           sprintf(label, "GEN%d", nr + 1);
> >>>>>>>>>           seq_printf(s, "%7s: %s, period:%llu duty:%d%% phase:%llu pol:%d",
> >>>>>>>>
> >>>>>>>> That's not correct, the dereference of bp->signal_out[nr] happens before
> >>>>>>>> the check. But I just wonder how can that even happen?
> >>>>>>>>
> >>>>>>> The scenario (our case): on ptp_ocp_adva_board_init we
> >>>>>>> initiate only signals 0 and 1 so 2 and 3 are NULL.
> >>>>>>> Later ptp_ocp_summary_show runs on all 4 signals and calls _signal_summary_show
> >>>>>>> when calling signal 2 or 3  the dereference occurs.
> >>>>>>> can you please explain: " the dereference of bp->signal_out[nr] happens before
> >>>>>>> the check", where exactly? do you mean in those lines:
> >>>>>>> struct signal_reg __iomem *reg = bp->signal_out[nr]->mem;
> >>>>>>        ^^^
> >>>>>> yes, this is the line which dereferences the pointer.
> >>>>>>
> >>>>>> but in case you have only 2 pins to configure, why the driver exposes 4
> >>>>>> SMAs? You can simply adjust the attributes (adva_timecard_attrs).
> >>>>>>
> >>>>> I can (and will) expose only 2 sma in adva_timecard_attrs, but still
> >>>>> ptp_ocp_summary_show runs
> >>>>> on all 4 signals and not only on the on that exposed, is it not a bug?
> >>>>
> >>>> Yeah, it's a bug, but different one, and we have to fix it other way.
> >>>>
> >>> Do you want to instruct me how to fix it , or will you fix it?
> >>
> >> well, the original device structure was not designed to have the amount
> >> of SMAs less than 4. We have to introduce another field to store actual
> >> amount of SMAs to work with, and adjust the code to check the value. The
> >> best solution would be to keep maximum amount of 4 SMAs in the structure
> >> but create a helper which will init new field and will have
> >> BUILD_BUG_ON() to prevent having more SMAs than fixed size array for
> >> them. That will solve your problem, but I will need to check it on the
> >> HW we run.
> >>
> > just to be clear you will write the fix and test it on your HW, so you
> > don't want me to write the fix?
>
> Well, it would be great if you can write the code which will make SMA
> functions flexible to the amount of pin the HW has. All our HW has fixed
> amount of 4 pins that's why the driver was coded with constants. Now
> your hardware has slightly different amount of pins, so it needs
> adjustments to the driver to work properly. I just want to be sure that
> any adjustments will not break my HW - that's what I meant saying I'll
> test it.
>
Just to be clear (correct me please if I am wrong):
I will write the code, then create a patch and upstream to the vanilla
you will test my change on your HW and only then approve the patch
> >>>>>>> struct ptp_ocp_signal *signal = &bp->signal[nr];
> >>>>>>>> I believe the proper fix is to move ptp_ocp_attr_group_add() closer to
> >>>>>>>> the end of ptp_ocp_adva_board_init() like it's done for other boards.
> >>>>>>>>
> >>>>>>>> --
> >>>>>>>> pw-bot: cr
> >>>>>>
> >>>>
> >>
>

Re: [PATCH v1] ptp: ocp: fix NULL deref in _signal_summary_show

[Vadim Fedorenko]

On 16/04/2025 14:59, Sagi Maimon wrote:
> On Wed, Apr 16, 2025 at 1:35 PM Vadim Fedorenko
> <vadim.fedorenko@linux.dev> wrote:
>>
>> On 16/04/2025 07:33, Sagi Maimon wrote:
>>> On Mon, Apr 14, 2025 at 4:55 PM Vadim Fedorenko
>>> <vadim.fedorenko@linux.dev> wrote:
>>>>
>>>> On 14/04/2025 14:43, Sagi Maimon wrote:
>>>>> On Mon, Apr 14, 2025 at 4:01 PM Vadim Fedorenko
>>>>> <vadim.fedorenko@linux.dev> wrote:
>>>>>>
>>>>>> On 14/04/2025 12:38, Sagi Maimon wrote:
>>>>>>> On Mon, Apr 14, 2025 at 2:09 PM Vadim Fedorenko
>>>>>>> <vadim.fedorenko@linux.dev> wrote:
>>>>>>>>
>>>>>>>> On 14/04/2025 11:56, Sagi Maimon wrote:
>>>>>>>>> On Mon, Apr 14, 2025 at 12:37 PM Vadim Fedorenko
>>>>>>>>> <vadim.fedorenko@linux.dev> wrote:
>>>>>>>>>>
>>>>>>>>>> On 14/04/2025 09:54, Sagi Maimon wrote:
>>>>>>>>>>> Sysfs signal show operations can invoke _signal_summary_show before
>>>>>>>>>>> signal_out array elements are initialized, causing a NULL pointer
>>>>>>>>>>> dereference. Add NULL checks for signal_out elements to prevent kernel
>>>>>>>>>>> crashes.
>>>>>>>>>>>
>>>>>>>>>>> Fixes: b325af3cfab9 ("ptp: ocp: Add signal generators and update sysfs nodes")
>>>>>>>>>>> Signed-off-by: Sagi Maimon <maimon.sagi@gmail.com>
>>>>>>>>>>> ---
>>>>>>>>>>>        drivers/ptp/ptp_ocp.c | 3 +++
>>>>>>>>>>>        1 file changed, 3 insertions(+)
>>>>>>>>>>>
>>>>>>>>>>> diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
>>>>>>>>>>> index 7945c6be1f7c..4c7893539cec 100644
>>>>>>>>>>> --- a/drivers/ptp/ptp_ocp.c
>>>>>>>>>>> +++ b/drivers/ptp/ptp_ocp.c
>>>>>>>>>>> @@ -3963,6 +3963,9 @@ _signal_summary_show(struct seq_file *s, struct ptp_ocp *bp, int nr)
>>>>>>>>>>>            bool on;
>>>>>>>>>>>            u32 val;
>>>>>>>>>>>
>>>>>>>>>>> +     if (!bp->signal_out[nr])
>>>>>>>>>>> +             return;
>>>>>>>>>>> +
>>>>>>>>>>>            on = signal->running;
>>>>>>>>>>>            sprintf(label, "GEN%d", nr + 1);
>>>>>>>>>>>            seq_printf(s, "%7s: %s, period:%llu duty:%d%% phase:%llu pol:%d",
>>>>>>>>>>
>>>>>>>>>> That's not correct, the dereference of bp->signal_out[nr] happens before
>>>>>>>>>> the check. But I just wonder how can that even happen?
>>>>>>>>>>
>>>>>>>>> The scenario (our case): on ptp_ocp_adva_board_init we
>>>>>>>>> initiate only signals 0 and 1 so 2 and 3 are NULL.
>>>>>>>>> Later ptp_ocp_summary_show runs on all 4 signals and calls _signal_summary_show
>>>>>>>>> when calling signal 2 or 3  the dereference occurs.
>>>>>>>>> can you please explain: " the dereference of bp->signal_out[nr] happens before
>>>>>>>>> the check", where exactly? do you mean in those lines:
>>>>>>>>> struct signal_reg __iomem *reg = bp->signal_out[nr]->mem;
>>>>>>>>         ^^^
>>>>>>>> yes, this is the line which dereferences the pointer.
>>>>>>>>
>>>>>>>> but in case you have only 2 pins to configure, why the driver exposes 4
>>>>>>>> SMAs? You can simply adjust the attributes (adva_timecard_attrs).
>>>>>>>>
>>>>>>> I can (and will) expose only 2 sma in adva_timecard_attrs, but still
>>>>>>> ptp_ocp_summary_show runs
>>>>>>> on all 4 signals and not only on the on that exposed, is it not a bug?
>>>>>>
>>>>>> Yeah, it's a bug, but different one, and we have to fix it other way.
>>>>>>
>>>>> Do you want to instruct me how to fix it , or will you fix it?
>>>>
>>>> well, the original device structure was not designed to have the amount
>>>> of SMAs less than 4. We have to introduce another field to store actual
>>>> amount of SMAs to work with, and adjust the code to check the value. The
>>>> best solution would be to keep maximum amount of 4 SMAs in the structure
>>>> but create a helper which will init new field and will have
>>>> BUILD_BUG_ON() to prevent having more SMAs than fixed size array for
>>>> them. That will solve your problem, but I will need to check it on the
>>>> HW we run.
>>>>
>>> just to be clear you will write the fix and test it on your HW, so you
>>> don't want me to write the fix?
>>
>> Well, it would be great if you can write the code which will make SMA
>> functions flexible to the amount of pin the HW has. All our HW has fixed
>> amount of 4 pins that's why the driver was coded with constants. Now
>> your hardware has slightly different amount of pins, so it needs
>> adjustments to the driver to work properly. I just want to be sure that
>> any adjustments will not break my HW - that's what I meant saying I'll
>> test it.
>>
> Just to be clear (correct me please if I am wrong):
> I will write the code, then create a patch and upstream to the vanilla
> you will test my change on your HW and only then approve the patch

Yes, that's correct

>>>>>>>>> struct ptp_ocp_signal *signal = &bp->signal[nr];
>>>>>>>>>> I believe the proper fix is to move ptp_ocp_attr_group_add() closer to
>>>>>>>>>> the end of ptp_ocp_adva_board_init() like it's done for other boards.
>>>>>>>>>>
>>>>>>>>>> --
>>>>>>>>>> pw-bot: cr
>>>>>>>>
>>>>>>
>>>>
>>

Re: [PATCH v1] ptp: ocp: fix NULL deref in _signal_summary_show

[Sagi Maimon]

On Wed, Apr 16, 2025 at 5:45 PM Vadim Fedorenko
<vadim.fedorenko@linux.dev> wrote:
>
> On 16/04/2025 14:59, Sagi Maimon wrote:
> > On Wed, Apr 16, 2025 at 1:35 PM Vadim Fedorenko
> > <vadim.fedorenko@linux.dev> wrote:
> >>
> >> On 16/04/2025 07:33, Sagi Maimon wrote:
> >>> On Mon, Apr 14, 2025 at 4:55 PM Vadim Fedorenko
> >>> <vadim.fedorenko@linux.dev> wrote:
> >>>>
> >>>> On 14/04/2025 14:43, Sagi Maimon wrote:
> >>>>> On Mon, Apr 14, 2025 at 4:01 PM Vadim Fedorenko
> >>>>> <vadim.fedorenko@linux.dev> wrote:
> >>>>>>
> >>>>>> On 14/04/2025 12:38, Sagi Maimon wrote:
> >>>>>>> On Mon, Apr 14, 2025 at 2:09 PM Vadim Fedorenko
> >>>>>>> <vadim.fedorenko@linux.dev> wrote:
> >>>>>>>>
> >>>>>>>> On 14/04/2025 11:56, Sagi Maimon wrote:
> >>>>>>>>> On Mon, Apr 14, 2025 at 12:37 PM Vadim Fedorenko
> >>>>>>>>> <vadim.fedorenko@linux.dev> wrote:
> >>>>>>>>>>
> >>>>>>>>>> On 14/04/2025 09:54, Sagi Maimon wrote:
> >>>>>>>>>>> Sysfs signal show operations can invoke _signal_summary_show before
> >>>>>>>>>>> signal_out array elements are initialized, causing a NULL pointer
> >>>>>>>>>>> dereference. Add NULL checks for signal_out elements to prevent kernel
> >>>>>>>>>>> crashes.
> >>>>>>>>>>>
> >>>>>>>>>>> Fixes: b325af3cfab9 ("ptp: ocp: Add signal generators and update sysfs nodes")
> >>>>>>>>>>> Signed-off-by: Sagi Maimon <maimon.sagi@gmail.com>
> >>>>>>>>>>> ---
> >>>>>>>>>>>        drivers/ptp/ptp_ocp.c | 3 +++
> >>>>>>>>>>>        1 file changed, 3 insertions(+)
> >>>>>>>>>>>
> >>>>>>>>>>> diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
> >>>>>>>>>>> index 7945c6be1f7c..4c7893539cec 100644
> >>>>>>>>>>> --- a/drivers/ptp/ptp_ocp.c
> >>>>>>>>>>> +++ b/drivers/ptp/ptp_ocp.c
> >>>>>>>>>>> @@ -3963,6 +3963,9 @@ _signal_summary_show(struct seq_file *s, struct ptp_ocp *bp, int nr)
> >>>>>>>>>>>            bool on;
> >>>>>>>>>>>            u32 val;
> >>>>>>>>>>>
> >>>>>>>>>>> +     if (!bp->signal_out[nr])
> >>>>>>>>>>> +             return;
> >>>>>>>>>>> +
> >>>>>>>>>>>            on = signal->running;
> >>>>>>>>>>>            sprintf(label, "GEN%d", nr + 1);
> >>>>>>>>>>>            seq_printf(s, "%7s: %s, period:%llu duty:%d%% phase:%llu pol:%d",
> >>>>>>>>>>
> >>>>>>>>>> That's not correct, the dereference of bp->signal_out[nr] happens before
> >>>>>>>>>> the check. But I just wonder how can that even happen?
> >>>>>>>>>>
> >>>>>>>>> The scenario (our case): on ptp_ocp_adva_board_init we
> >>>>>>>>> initiate only signals 0 and 1 so 2 and 3 are NULL.
> >>>>>>>>> Later ptp_ocp_summary_show runs on all 4 signals and calls _signal_summary_show
> >>>>>>>>> when calling signal 2 or 3  the dereference occurs.
> >>>>>>>>> can you please explain: " the dereference of bp->signal_out[nr] happens before
> >>>>>>>>> the check", where exactly? do you mean in those lines:
> >>>>>>>>> struct signal_reg __iomem *reg = bp->signal_out[nr]->mem;
> >>>>>>>>         ^^^
> >>>>>>>> yes, this is the line which dereferences the pointer.
> >>>>>>>>
> >>>>>>>> but in case you have only 2 pins to configure, why the driver exposes 4
> >>>>>>>> SMAs? You can simply adjust the attributes (adva_timecard_attrs).
> >>>>>>>>
> >>>>>>> I can (and will) expose only 2 sma in adva_timecard_attrs, but still
> >>>>>>> ptp_ocp_summary_show runs
> >>>>>>> on all 4 signals and not only on the on that exposed, is it not a bug?
> >>>>>>
> >>>>>> Yeah, it's a bug, but different one, and we have to fix it other way.
> >>>>>>
> >>>>> Do you want to instruct me how to fix it , or will you fix it?
> >>>>
> >>>> well, the original device structure was not designed to have the amount
> >>>> of SMAs less than 4. We have to introduce another field to store actual
> >>>> amount of SMAs to work with, and adjust the code to check the value. The
> >>>> best solution would be to keep maximum amount of 4 SMAs in the structure
> >>>> but create a helper which will init new field and will have
> >>>> BUILD_BUG_ON() to prevent having more SMAs than fixed size array for
> >>>> them. That will solve your problem, but I will need to check it on the
> >>>> HW we run.
> >>>>
> >>> just to be clear you will write the fix and test it on your HW, so you
> >>> don't want me to write the fix?
> >>
> >> Well, it would be great if you can write the code which will make SMA
> >> functions flexible to the amount of pin the HW has. All our HW has fixed
> >> amount of 4 pins that's why the driver was coded with constants. Now
> >> your hardware has slightly different amount of pins, so it needs
> >> adjustments to the driver to work properly. I just want to be sure that
> >> any adjustments will not break my HW - that's what I meant saying I'll
> >> test it.
> >>
> > Just to be clear (correct me please if I am wrong):
> > I will write the code, then create a patch and upstream to the vanilla
> > you will test my change on your HW and only then approve the patch
>
> Yes, that's correct
>
On altera we have implemented 2 signals and 4 SMAs (does not make sense, but...)
The original fix is regarding struct ptp_ocp_signal signal[4], but on
your fix suggestion
you mension SMAs.
So what to do? fix the SMA array or signal array or both?
and if both should we establish some connection between the two
meaning if we have only
two SMAs then we can initiate only two signals?
please advise
> >>>>>>>>> struct ptp_ocp_signal *signal = &bp->signal[nr];
> >>>>>>>>>> I believe the proper fix is to move ptp_ocp_attr_group_add() closer to
> >>>>>>>>>> the end of ptp_ocp_adva_board_init() like it's done for other boards.
> >>>>>>>>>>
> >>>>>>>>>> --
> >>>>>>>>>> pw-bot: cr
> >>>>>>>>
> >>>>>>
> >>>>
> >>
>

Re: [PATCH v1] ptp: ocp: fix NULL deref in _signal_summary_show

[Vadim Fedorenko]

On 29/04/2025 09:42, Sagi Maimon wrote:
> On Wed, Apr 16, 2025 at 5:45 PM Vadim Fedorenko
> <vadim.fedorenko@linux.dev> wrote:
>>
>> On 16/04/2025 14:59, Sagi Maimon wrote:
>>> On Wed, Apr 16, 2025 at 1:35 PM Vadim Fedorenko
>>> <vadim.fedorenko@linux.dev> wrote:
>>>>
>>>> On 16/04/2025 07:33, Sagi Maimon wrote:
>>>>> On Mon, Apr 14, 2025 at 4:55 PM Vadim Fedorenko
>>>>> <vadim.fedorenko@linux.dev> wrote:
>>>>>>
>>>>>> On 14/04/2025 14:43, Sagi Maimon wrote:
>>>>>>> On Mon, Apr 14, 2025 at 4:01 PM Vadim Fedorenko
>>>>>>> <vadim.fedorenko@linux.dev> wrote:
>>>>>>>>
>>>>>>>> On 14/04/2025 12:38, Sagi Maimon wrote:
>>>>>>>>> On Mon, Apr 14, 2025 at 2:09 PM Vadim Fedorenko
>>>>>>>>> <vadim.fedorenko@linux.dev> wrote:
>>>>>>>>>>
>>>>>>>>>> On 14/04/2025 11:56, Sagi Maimon wrote:
>>>>>>>>>>> On Mon, Apr 14, 2025 at 12:37 PM Vadim Fedorenko
>>>>>>>>>>> <vadim.fedorenko@linux.dev> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>> On 14/04/2025 09:54, Sagi Maimon wrote:
>>>>>>>>>>>>> Sysfs signal show operations can invoke _signal_summary_show before
>>>>>>>>>>>>> signal_out array elements are initialized, causing a NULL pointer
>>>>>>>>>>>>> dereference. Add NULL checks for signal_out elements to prevent kernel
>>>>>>>>>>>>> crashes.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Fixes: b325af3cfab9 ("ptp: ocp: Add signal generators and update sysfs nodes")
>>>>>>>>>>>>> Signed-off-by: Sagi Maimon <maimon.sagi@gmail.com>
>>>>>>>>>>>>> ---
>>>>>>>>>>>>>         drivers/ptp/ptp_ocp.c | 3 +++
>>>>>>>>>>>>>         1 file changed, 3 insertions(+)
>>>>>>>>>>>>>
>>>>>>>>>>>>> diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
>>>>>>>>>>>>> index 7945c6be1f7c..4c7893539cec 100644
>>>>>>>>>>>>> --- a/drivers/ptp/ptp_ocp.c
>>>>>>>>>>>>> +++ b/drivers/ptp/ptp_ocp.c
>>>>>>>>>>>>> @@ -3963,6 +3963,9 @@ _signal_summary_show(struct seq_file *s, struct ptp_ocp *bp, int nr)
>>>>>>>>>>>>>             bool on;
>>>>>>>>>>>>>             u32 val;
>>>>>>>>>>>>>
>>>>>>>>>>>>> +     if (!bp->signal_out[nr])
>>>>>>>>>>>>> +             return;
>>>>>>>>>>>>> +
>>>>>>>>>>>>>             on = signal->running;
>>>>>>>>>>>>>             sprintf(label, "GEN%d", nr + 1);
>>>>>>>>>>>>>             seq_printf(s, "%7s: %s, period:%llu duty:%d%% phase:%llu pol:%d",
>>>>>>>>>>>>
>>>>>>>>>>>> That's not correct, the dereference of bp->signal_out[nr] happens before
>>>>>>>>>>>> the check. But I just wonder how can that even happen?
>>>>>>>>>>>>
>>>>>>>>>>> The scenario (our case): on ptp_ocp_adva_board_init we
>>>>>>>>>>> initiate only signals 0 and 1 so 2 and 3 are NULL.
>>>>>>>>>>> Later ptp_ocp_summary_show runs on all 4 signals and calls _signal_summary_show
>>>>>>>>>>> when calling signal 2 or 3  the dereference occurs.
>>>>>>>>>>> can you please explain: " the dereference of bp->signal_out[nr] happens before
>>>>>>>>>>> the check", where exactly? do you mean in those lines:
>>>>>>>>>>> struct signal_reg __iomem *reg = bp->signal_out[nr]->mem;
>>>>>>>>>>          ^^^
>>>>>>>>>> yes, this is the line which dereferences the pointer.
>>>>>>>>>>
>>>>>>>>>> but in case you have only 2 pins to configure, why the driver exposes 4
>>>>>>>>>> SMAs? You can simply adjust the attributes (adva_timecard_attrs).
>>>>>>>>>>
>>>>>>>>> I can (and will) expose only 2 sma in adva_timecard_attrs, but still
>>>>>>>>> ptp_ocp_summary_show runs
>>>>>>>>> on all 4 signals and not only on the on that exposed, is it not a bug?
>>>>>>>>
>>>>>>>> Yeah, it's a bug, but different one, and we have to fix it other way.
>>>>>>>>
>>>>>>> Do you want to instruct me how to fix it , or will you fix it?
>>>>>>
>>>>>> well, the original device structure was not designed to have the amount
>>>>>> of SMAs less than 4. We have to introduce another field to store actual
>>>>>> amount of SMAs to work with, and adjust the code to check the value. The
>>>>>> best solution would be to keep maximum amount of 4 SMAs in the structure
>>>>>> but create a helper which will init new field and will have
>>>>>> BUILD_BUG_ON() to prevent having more SMAs than fixed size array for
>>>>>> them. That will solve your problem, but I will need to check it on the
>>>>>> HW we run.
>>>>>>
>>>>> just to be clear you will write the fix and test it on your HW, so you
>>>>> don't want me to write the fix?
>>>>
>>>> Well, it would be great if you can write the code which will make SMA
>>>> functions flexible to the amount of pin the HW has. All our HW has fixed
>>>> amount of 4 pins that's why the driver was coded with constants. Now
>>>> your hardware has slightly different amount of pins, so it needs
>>>> adjustments to the driver to work properly. I just want to be sure that
>>>> any adjustments will not break my HW - that's what I meant saying I'll
>>>> test it.
>>>>
>>> Just to be clear (correct me please if I am wrong):
>>> I will write the code, then create a patch and upstream to the vanilla
>>> you will test my change on your HW and only then approve the patch
>>
>> Yes, that's correct
>>
> On altera we have implemented 2 signals and 4 SMAs (does not make sense, but...)
> The original fix is regarding struct ptp_ocp_signal signal[4], but on
> your fix suggestion
> you mension SMAs.
> So what to do? fix the SMA array or signal array or both?
> and if both should we establish some connection between the two
> meaning if we have only
> two SMAs then we can initiate only two signals?
> please advise

Ok, now I got it. Previously, the ptp_ocp driver assumed that the amount
of SMAs is equal to the amount signals supported as well as frequency
outputs and always equals to 4. With the details you provided the
assumption is wrong and we have to make proper code for such cases.

I would suggest to introduce 3 fields of u8 which will store the real
amount of SMAs/signals/frequencies supported by probed hardware. And
reuse this fields in any show/set functions.

>>>>>>>>>>> struct ptp_ocp_signal *signal = &bp->signal[nr];
>>>>>>>>>>>> I believe the proper fix is to move ptp_ocp_attr_group_add() closer to
>>>>>>>>>>>> the end of ptp_ocp_adva_board_init() like it's done for other boards.
>>>>>>>>>>>>
>>>>>>>>>>>> --
>>>>>>>>>>>> pw-bot: cr
>>>>>>>>>>
>>>>>>>>
>>>>>>
>>>>
>>