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

[Vadim Fedorenko <vadim.fedorenko@linux.dev>]

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