| Message ID | 20250414085412.117120-1-maimon.sagi@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v1] ptp: ocp: fix NULL deref in _signal_summary_show | expand |
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
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
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
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 >
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 >>
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 > >> >
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 >>>> >>
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 > >>>> > >> >
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 >>>>>> >>>> >>
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",
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(+)