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

From: Vadim Fedorenko <[email-masked]>
Date: Wed, 30 Apr 2025 10:52:24 +0100

On 29/04/2025 09:42, Sagi Maimon wrote:
> On Wed, Apr 16, 2025 at 5:45 PM Vadim Fedorenko
> <[email-masked]> wrote:
>>
>> On 16/04/2025 14:59, Sagi Maimon wrote:
>>> On Wed, Apr 16, 2025 at 1:35 PM Vadim Fedorenko
>>> <[email-masked]> wrote:
>>>>
>>>> On 16/04/2025 07:33, Sagi Maimon wrote:
>>>>> On Mon, Apr 14, 2025 at 4:55 PM Vadim Fedorenko
>>>>> <[email-masked]> wrote:
>>>>>>
>>>>>> On 14/04/2025 14:43, Sagi Maimon wrote:
>>>>>>> On Mon, Apr 14, 2025 at 4:01 PM Vadim Fedorenko
>>>>>>> <[email-masked]> wrote:
>>>>>>>>
>>>>>>>> On 14/04/2025 12:38, Sagi Maimon wrote:
>>>>>>>>> On Mon, Apr 14, 2025 at 2:09 PM Vadim Fedorenko
>>>>>>>>> <[email-masked]> wrote:
>>>>>>>>>>
>>>>>>>>>> On 14/04/2025 11:56, Sagi Maimon wrote:
>>>>>>>>>>> On Mon, Apr 14, 2025 at 12:37 PM Vadim Fedorenko
>>>>>>>>>>> <[email-masked]> 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 <[email-masked]>
>>>>>>>>>>>>> ---
>>>>>>>>>>>>>         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
>>>>>>>>>>
>>>>>>>>
>>>>>>
>>>>
>>