| Message ID | 20250415064638.130453-1-maimon.sagi@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v2] ptp: ocp: fix NULL deref in __handle_s for irig/dcf | expand |
On Tue, Apr 15, 2025 at 09:46:38AM +0300, Sagi Maimon wrote: > SMA store/get operations via sysfs can call __handle_signal_outputs > or __handle_signal_inputs while irig and dcf pointers remain > uninitialized. This leads to a NULL pointer dereference in > __handle_s. Add NULL checks for irig and dcf to prevent crashes. > > Fixes: b286f4e87e32 ("serial: core: Move tty and serdev to be children of serial core port device") > Reviewed-by: Vadim Fedorenko <vadim.fedorenko@linux.dev> > Signed-off-by: Sagi Maimon <maimon.sagi@gmail.com> > --- > Addressed comments from Paolo Abeni: > - https://www.spinics.net/lists/netdev/msg1082406.html > Changes since v1: > - Expanded commit message to clarify the NULL dereference scenario. > --- > drivers/ptp/ptp_ocp.c | 12 ++++++++---- > 1 file changed, 8 insertions(+), 4 deletions(-) > > diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c > index 7945c6be1f7c..4e4a6f465b01 100644 > --- a/drivers/ptp/ptp_ocp.c > +++ b/drivers/ptp/ptp_ocp.c > @@ -2434,15 +2434,19 @@ ptp_ocp_dcf_in(struct ptp_ocp *bp, bool enable) > static void > __handle_signal_outputs(struct ptp_ocp *bp, u32 val) > { > - ptp_ocp_irig_out(bp, val & 0x00100010); > - ptp_ocp_dcf_out(bp, val & 0x00200020); > + if (bp->irig_out) > + ptp_ocp_irig_out(bp, val & 0x00100010); > + if (bp->dcf_out) > + ptp_ocp_dcf_out(bp, val & 0x00200020); > } > > static void > __handle_signal_inputs(struct ptp_ocp *bp, u32 val) > { > - ptp_ocp_irig_in(bp, val & 0x00100010); > - ptp_ocp_dcf_in(bp, val & 0x00200020); > + if (bp->irig_out) Why not irig_in? Can we asssume that "in" isn't NULL if "out" isn't? > + ptp_ocp_irig_in(bp, val & 0x00100010); > + if (bp->dcf_out) The same here. > + ptp_ocp_dcf_in(bp, val & 0x00200020); Just my opinion, I will move these checks into ptp_ocp_...() functions as bp is passed to a function not bp->sth. > } > > static u32 > -- > 2.47.0
On Tue, Apr 15, 2025 at 10:00 AM Michal Swiatkowski <michal.swiatkowski@linux.intel.com> wrote: > > On Tue, Apr 15, 2025 at 09:46:38AM +0300, Sagi Maimon wrote: > > SMA store/get operations via sysfs can call __handle_signal_outputs > > or __handle_signal_inputs while irig and dcf pointers remain > > uninitialized. This leads to a NULL pointer dereference in > > __handle_s. Add NULL checks for irig and dcf to prevent crashes. > > > > Fixes: b286f4e87e32 ("serial: core: Move tty and serdev to be children of serial core port device") > > Reviewed-by: Vadim Fedorenko <vadim.fedorenko@linux.dev> > > Signed-off-by: Sagi Maimon <maimon.sagi@gmail.com> > > --- > > Addressed comments from Paolo Abeni: > > - https://www.spinics.net/lists/netdev/msg1082406.html > > Changes since v1: > > - Expanded commit message to clarify the NULL dereference scenario. > > --- > > drivers/ptp/ptp_ocp.c | 12 ++++++++---- > > 1 file changed, 8 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c > > index 7945c6be1f7c..4e4a6f465b01 100644 > > --- a/drivers/ptp/ptp_ocp.c > > +++ b/drivers/ptp/ptp_ocp.c > > @@ -2434,15 +2434,19 @@ ptp_ocp_dcf_in(struct ptp_ocp *bp, bool enable) > > static void > > __handle_signal_outputs(struct ptp_ocp *bp, u32 val) > > { > > - ptp_ocp_irig_out(bp, val & 0x00100010); > > - ptp_ocp_dcf_out(bp, val & 0x00200020); > > + if (bp->irig_out) > > + ptp_ocp_irig_out(bp, val & 0x00100010); > > + if (bp->dcf_out) > > + ptp_ocp_dcf_out(bp, val & 0x00200020); > > } > > > > static void > > __handle_signal_inputs(struct ptp_ocp *bp, u32 val) > > { > > - ptp_ocp_irig_in(bp, val & 0x00100010); > > - ptp_ocp_dcf_in(bp, val & 0x00200020); > > + if (bp->irig_out) > Why not irig_in? Can we asssume that "in" isn't NULL if "out" isn't? > Ss part of ocp_resource initiation, irig_in and irig_out initiated separately (one can be initiated and the other not ) so we can't assume that > > + ptp_ocp_irig_in(bp, val & 0x00100010); > > + if (bp->dcf_out) > The same here. > > > + ptp_ocp_dcf_in(bp, val & 0x00200020); > > Just my opinion, I will move these checks into ptp_ocp_...() functions > as bp is passed to a function not bp->sth. > > > } > > > > static u32 > > -- > > 2.47.0
On 15/04/2025 07:46, Sagi Maimon wrote: > SMA store/get operations via sysfs can call __handle_signal_outputs > or __handle_signal_inputs while irig and dcf pointers remain > uninitialized. This leads to a NULL pointer dereference in > __handle_s. Add NULL checks for irig and dcf to prevent crashes. Ok, looks like I misread the patch previously. IRIG and DCF registers are mapped unconditionally for OCP TimeCard. The functions you are trying to fix are HW-specific functions which you decided to reuse for your hardware. If your hardware doesn't support this function, you have to implement your own HW-specific callbacks used in ocp_adva_sma_op structure. I have to take back my Rb tag, and for this patch it's definitely NAck. > > Fixes: b286f4e87e32 ("serial: core: Move tty and serdev to be children of serial core port device") > Reviewed-by: Vadim Fedorenko <vadim.fedorenko@linux.dev> > Signed-off-by: Sagi Maimon <maimon.sagi@gmail.com> > --- > Addressed comments from Paolo Abeni: > - https://www.spinics.net/lists/netdev/msg1082406.html > Changes since v1: > - Expanded commit message to clarify the NULL dereference scenario. > --- > drivers/ptp/ptp_ocp.c | 12 ++++++++---- > 1 file changed, 8 insertions(+), 4 deletions(-) > > diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c > index 7945c6be1f7c..4e4a6f465b01 100644 > --- a/drivers/ptp/ptp_ocp.c > +++ b/drivers/ptp/ptp_ocp.c > @@ -2434,15 +2434,19 @@ ptp_ocp_dcf_in(struct ptp_ocp *bp, bool enable) > static void > __handle_signal_outputs(struct ptp_ocp *bp, u32 val) > { > - ptp_ocp_irig_out(bp, val & 0x00100010); > - ptp_ocp_dcf_out(bp, val & 0x00200020); > + if (bp->irig_out) > + ptp_ocp_irig_out(bp, val & 0x00100010); > + if (bp->dcf_out) > + ptp_ocp_dcf_out(bp, val & 0x00200020); > } > > static void > __handle_signal_inputs(struct ptp_ocp *bp, u32 val) > { > - ptp_ocp_irig_in(bp, val & 0x00100010); > - ptp_ocp_dcf_in(bp, val & 0x00200020); > + if (bp->irig_out) > + ptp_ocp_irig_in(bp, val & 0x00100010); > + if (bp->dcf_out) > + ptp_ocp_dcf_in(bp, val & 0x00200020); > } > > static u32
diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c index 7945c6be1f7c..4e4a6f465b01 100644 --- a/drivers/ptp/ptp_ocp.c +++ b/drivers/ptp/ptp_ocp.c @@ -2434,15 +2434,19 @@ ptp_ocp_dcf_in(struct ptp_ocp *bp, bool enable) static void __handle_signal_outputs(struct ptp_ocp *bp, u32 val) { - ptp_ocp_irig_out(bp, val & 0x00100010); - ptp_ocp_dcf_out(bp, val & 0x00200020); + if (bp->irig_out) + ptp_ocp_irig_out(bp, val & 0x00100010); + if (bp->dcf_out) + ptp_ocp_dcf_out(bp, val & 0x00200020); } static void __handle_signal_inputs(struct ptp_ocp *bp, u32 val) { - ptp_ocp_irig_in(bp, val & 0x00100010); - ptp_ocp_dcf_in(bp, val & 0x00200020); + if (bp->irig_out) + ptp_ocp_irig_in(bp, val & 0x00100010); + if (bp->dcf_out) + ptp_ocp_dcf_in(bp, val & 0x00200020); } static u32
SMA store/get operations via sysfs can call __handle_signal_outputs or __handle_signal_inputs while irig and dcf pointers remain uninitialized. This leads to a NULL pointer dereference in __handle_s. Add NULL checks for irig and dcf to prevent crashes. Fixes: b286f4e87e32 ("serial: core: Move tty and serdev to be children of serial core port device") Reviewed-by: Vadim Fedorenko <vadim.fedorenko@linux.dev> Signed-off-by: Sagi Maimon <maimon.sagi@gmail.com> --- Addressed comments from Paolo Abeni: - https://www.spinics.net/lists/netdev/msg1082406.html Changes since v1: - Expanded commit message to clarify the NULL dereference scenario. --- drivers/ptp/ptp_ocp.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)