Fix two issues in fsl_lpuart.c reported by Coverity Scan.
Sherry Sun (2):
tty: serial: fsl_lpuart: fix the potential bug of division or modulo
by zero
tty: serial: fsl_lpuart: fix the potential bug of dereference null
return value
drivers/tty/serial/fsl_lpuart.c | 6 ++++++
1 file changed, 6 insertions(+)
--
2.17.1
This issue is reported by Coverity Check.
In lpuart32_console_get_options, division or modulo by zero may results
in undefined behavior.
Signed-off-by: Sherry Sun <[email protected]>
---
drivers/tty/serial/fsl_lpuart.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c
index 794035041744..777d54b593f8 100644
--- a/drivers/tty/serial/fsl_lpuart.c
+++ b/drivers/tty/serial/fsl_lpuart.c
@@ -2414,6 +2414,9 @@ lpuart32_console_get_options(struct lpuart_port *sport, int *baud,
bd = lpuart32_read(&sport->port, UARTBAUD);
bd &= UARTBAUD_SBR_MASK;
+ if (!bd)
+ return;
+
sbr = bd;
uartclk = lpuart_get_baud_clk_rate(sport);
/*
--
2.17.1
This issue is reported by Coverity Check.
In lpuart_probe, return value of function which returns null is
dereferenced without checking.
Signed-off-by: Sherry Sun <[email protected]>
---
drivers/tty/serial/fsl_lpuart.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c
index 777d54b593f8..c95e71fd2ca0 100644
--- a/drivers/tty/serial/fsl_lpuart.c
+++ b/drivers/tty/serial/fsl_lpuart.c
@@ -2589,6 +2589,9 @@ static int lpuart_probe(struct platform_device *pdev)
struct resource *res;
int ret;
+ if (!sdata)
+ return -ENODEV;
+
sport = devm_kzalloc(&pdev->dev, sizeof(*sport), GFP_KERNEL);
if (!sport)
return -ENOMEM;
--
2.17.1
On Mon, Apr 26, 2021 at 03:49:34PM +0800, Sherry Sun wrote:
> This issue is reported by Coverity Check.
> In lpuart32_console_get_options, division or modulo by zero may results
> in undefined behavior.
>
> Signed-off-by: Sherry Sun <[email protected]>
> ---
> drivers/tty/serial/fsl_lpuart.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c
> index 794035041744..777d54b593f8 100644
> --- a/drivers/tty/serial/fsl_lpuart.c
> +++ b/drivers/tty/serial/fsl_lpuart.c
> @@ -2414,6 +2414,9 @@ lpuart32_console_get_options(struct lpuart_port *sport, int *baud,
>
> bd = lpuart32_read(&sport->port, UARTBAUD);
> bd &= UARTBAUD_SBR_MASK;
> + if (!bd)
> + return;
How can this ever happen?
Not to say this is a bad check, but it feels like this can't really
happen in real life, what code patch could create this result?
And have you tested this on real hardware?
thanks,
greg k-h
On Mon, Apr 26, 2021 at 03:49:35PM +0800, Sherry Sun wrote:
> This issue is reported by Coverity Check.
> In lpuart_probe, return value of function which returns null is
> dereferenced without checking.
>
> Signed-off-by: Sherry Sun <[email protected]>
> ---
> drivers/tty/serial/fsl_lpuart.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c
> index 777d54b593f8..c95e71fd2ca0 100644
> --- a/drivers/tty/serial/fsl_lpuart.c
> +++ b/drivers/tty/serial/fsl_lpuart.c
> @@ -2589,6 +2589,9 @@ static int lpuart_probe(struct platform_device *pdev)
> struct resource *res;
> int ret;
>
> + if (!sdata)
> + return -ENODEV;
How can sdata be NULL?
thanks,
greg k-h
Hi Greg,
> -----Original Message-----
> From: Greg KH <[email protected]>
> Sent: 2021??4??26?? 16:09
> To: Sherry Sun <[email protected]>
> Cc: [email protected]; [email protected]; linux-
> [email protected]; dl-linux-imx <[email protected]>
> Subject: Re: [PATCH 1/2] tty: serial: fsl_lpuart: fix the potential bug of division
> or modulo by zero
>
> On Mon, Apr 26, 2021 at 03:49:34PM +0800, Sherry Sun wrote:
> > This issue is reported by Coverity Check.
> > In lpuart32_console_get_options, division or modulo by zero may
> > results in undefined behavior.
> >
> > Signed-off-by: Sherry Sun <[email protected]>
> > ---
> > drivers/tty/serial/fsl_lpuart.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/tty/serial/fsl_lpuart.c
> > b/drivers/tty/serial/fsl_lpuart.c index 794035041744..777d54b593f8
> > 100644
> > --- a/drivers/tty/serial/fsl_lpuart.c
> > +++ b/drivers/tty/serial/fsl_lpuart.c
> > @@ -2414,6 +2414,9 @@ lpuart32_console_get_options(struct lpuart_port
> > *sport, int *baud,
> >
> > bd = lpuart32_read(&sport->port, UARTBAUD);
> > bd &= UARTBAUD_SBR_MASK;
> > + if (!bd)
> > + return;
>
> How can this ever happen?
>
> Not to say this is a bad check, but it feels like this can't really happen in real
> life, what code patch could create this result?
>
> And have you tested this on real hardware?
>
Thanks for the reviewing, yes, I have tested the patchset on the real hardware.
Seems the coverity check is static scan, so cannot judge if UARTBAUD Register will be zero.
I just found below statement in the uart reference manual: "When SBR is 1 - 8191, the baud rate equals "baud clock / ((OSR+1) ?? SBR)"."
Since I am not familiar with uart, do you mean that the value of UARTBAUD Register will never be zero, so this case will not happen in real word?
If yes, I will drop this patch.
Best regards
Sherry
> thanks,
>
> greg k-h
On Mon, Apr 26, 2021 at 11:30:47AM +0000, Sherry Sun wrote:
> Hi Greg,
>
> > -----Original Message-----
> > From: Greg KH <[email protected]>
> > Sent: 2021年4月26日 16:09
> > To: Sherry Sun <[email protected]>
> > Cc: [email protected]; [email protected]; linux-
> > [email protected]; dl-linux-imx <[email protected]>
> > Subject: Re: [PATCH 1/2] tty: serial: fsl_lpuart: fix the potential bug of division
> > or modulo by zero
> >
> > On Mon, Apr 26, 2021 at 03:49:34PM +0800, Sherry Sun wrote:
> > > This issue is reported by Coverity Check.
> > > In lpuart32_console_get_options, division or modulo by zero may
> > > results in undefined behavior.
> > >
> > > Signed-off-by: Sherry Sun <[email protected]>
> > > ---
> > > drivers/tty/serial/fsl_lpuart.c | 3 +++
> > > 1 file changed, 3 insertions(+)
> > >
> > > diff --git a/drivers/tty/serial/fsl_lpuart.c
> > > b/drivers/tty/serial/fsl_lpuart.c index 794035041744..777d54b593f8
> > > 100644
> > > --- a/drivers/tty/serial/fsl_lpuart.c
> > > +++ b/drivers/tty/serial/fsl_lpuart.c
> > > @@ -2414,6 +2414,9 @@ lpuart32_console_get_options(struct lpuart_port
> > > *sport, int *baud,
> > >
> > > bd = lpuart32_read(&sport->port, UARTBAUD);
> > > bd &= UARTBAUD_SBR_MASK;
> > > + if (!bd)
> > > + return;
> >
> > How can this ever happen?
> >
> > Not to say this is a bad check, but it feels like this can't really happen in real
> > life, what code patch could create this result?
> >
> > And have you tested this on real hardware?
> >
>
> Thanks for the reviewing, yes, I have tested the patchset on the real hardware.
>
> Seems the coverity check is static scan, so cannot judge if UARTBAUD Register will be zero.
> I just found below statement in the uart reference manual: "When SBR is 1 - 8191, the baud rate equals "baud clock / ((OSR+1) × SBR)"."
> Since I am not familiar with uart, do you mean that the value of UARTBAUD Register will never be zero, so this case will not happen in real word?
Given that this never has happened with hardware for such an old device,
perhaps it is impossible. But it would be good to check.
> If yes, I will drop this patch.
Handling "bad data" from hardware is never a bad idea, so I don't
necessarily want to drop this patch, I just want to try to figure out if
this is a "incase the hardware is broken/malicious" type of change, vs.
a "this bug we are seeing in real hardware" type of change.
thanks,
greg k-h
Hi Greg,
> -----Original Message-----
> From: Greg KH <[email protected]>
> Sent: 2021??4??26?? 16:09
> To: Sherry Sun <[email protected]>
> Cc: [email protected]; [email protected]; linux-
> [email protected]; dl-linux-imx <[email protected]>
> Subject: Re: [PATCH 2/2] tty: serial: fsl_lpuart: fix the potential bug of
> dereference null return value
>
> On Mon, Apr 26, 2021 at 03:49:35PM +0800, Sherry Sun wrote:
> > This issue is reported by Coverity Check.
> > In lpuart_probe, return value of function which returns null is
> > dereferenced without checking.
> >
> > Signed-off-by: Sherry Sun <[email protected]>
> > ---
> > drivers/tty/serial/fsl_lpuart.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/tty/serial/fsl_lpuart.c
> > b/drivers/tty/serial/fsl_lpuart.c index 777d54b593f8..c95e71fd2ca0
> > 100644
> > --- a/drivers/tty/serial/fsl_lpuart.c
> > +++ b/drivers/tty/serial/fsl_lpuart.c
> > @@ -2589,6 +2589,9 @@ static int lpuart_probe(struct platform_device
> *pdev)
> > struct resource *res;
> > int ret;
> >
> > + if (!sdata)
> > + return -ENODEV;
>
> How can sdata be NULL?
Is it possible that a case forgot to set sdata? Then the value will be NULL, such as { .compatible = "fsl,imx8qxp-lpuart", }.
So I added the patch to avoid the kernel crash when run to sdata->reg_off directly. But I am not sure does it make sense.
Thanks again for your time.
Best regards
Sherry
>
> thanks,
>
> greg k-h
Hi Greg,
> > > > drivers/tty/serial/fsl_lpuart.c | 3 +++
> > > > 1 file changed, 3 insertions(+)
> > > >
> > > > diff --git a/drivers/tty/serial/fsl_lpuart.c
> > > > b/drivers/tty/serial/fsl_lpuart.c index 794035041744..777d54b593f8
> > > > 100644
> > > > --- a/drivers/tty/serial/fsl_lpuart.c
> > > > +++ b/drivers/tty/serial/fsl_lpuart.c
> > > > @@ -2414,6 +2414,9 @@ lpuart32_console_get_options(struct
> > > > lpuart_port *sport, int *baud,
> > > >
> > > > bd = lpuart32_read(&sport->port, UARTBAUD);
> > > > bd &= UARTBAUD_SBR_MASK;
> > > > + if (!bd)
> > > > + return;
> > >
> > > How can this ever happen?
> > >
> > > Not to say this is a bad check, but it feels like this can't really
> > > happen in real life, what code patch could create this result?
> > >
> > > And have you tested this on real hardware?
> > >
> >
> > Thanks for the reviewing, yes, I have tested the patchset on the real
> hardware.
> >
> > Seems the coverity check is static scan, so cannot judge if UARTBAUD
> Register will be zero.
> > I just found below statement in the uart reference manual: "When SBR is 1
> - 8191, the baud rate equals "baud clock / ((OSR+1) × SBR)"."
> > Since I am not familiar with uart, do you mean that the value of UARTBAUD
> Register will never be zero, so this case will not happen in real word?
>
> Given that this never has happened with hardware for such an old device,
> perhaps it is impossible. But it would be good to check.
>
> > If yes, I will drop this patch.
>
> Handling "bad data" from hardware is never a bad idea, so I don't
> necessarily want to drop this patch, I just want to try to figure out if this is a
> "incase the hardware is broken/malicious" type of change, vs.
> a "this bug we are seeing in real hardware" type of change.
>
Yes, you are right, the probability of hardware happen in this case is really low. But we cannot guarantee that it will never happen.
So will this check here be accepted? Thanks!
Best regards
Sherry
> thanks,
>
> greg k-h
Hi Sherry,
On Mon, Apr 26, 2021 at 8:39 AM Sherry Sun <[email protected]> wrote:
> > > + if (!sdata)
> > > + return -ENODEV;
> >
> > How can sdata be NULL?
>
> Is it possible that a case forgot to set sdata? Then the value will be NULL, such as { .compatible = "fsl,imx8qxp-lpuart", }.
> So I added the patch to avoid the kernel crash when run to sdata->reg_off directly. But I am not sure does it make sense.
sdata comes directly from of_device_get_match_data().
This driver only runs on DT platforms and the only way of being probed
is when a compatible string matches, so it is not possible that sdata
can be NULL.
Hi Fabio,
> Hi Sherry,
>
> On Mon, Apr 26, 2021 at 8:39 AM Sherry Sun <[email protected]> wrote:
>
> > > > + if (!sdata)
> > > > + return -ENODEV;
> > >
> > > How can sdata be NULL?
> >
> > Is it possible that a case forgot to set sdata? Then the value will be NULL,
> such as { .compatible = "fsl,imx8qxp-lpuart", }.
> > So I added the patch to avoid the kernel crash when run to sdata->reg_off
> directly. But I am not sure does it make sense.
>
> sdata comes directly from of_device_get_match_data().
>
> This driver only runs on DT platforms and the only way of being probed is
> when a compatible string matches, so it is not possible that sdata can be
> NULL.
Thanks for your reply.
I guess you mean the of_match_table will not be NULL since it contains compatible, right?
But for the lpuart data -- struct lpuart_soc_data, won’t it meet the NULL case? such as { .compatible = "fsl,imx8qxp-lpuart", }.
Here of_device_id won’t be NULL, but lpuart_soc_data Is NULL.
Best regards
Sherry
On Mon, Apr 26, 2021 at 9:09 AM Sherry Sun <[email protected]> wrote:
> Thanks for your reply.
> I guess you mean the of_match_table will not be NULL since it contains compatible, right?
> But for the lpuart data -- struct lpuart_soc_data, won’t it meet the NULL case? such as { .compatible = "fsl,imx8qxp-lpuart", }.
> Here of_device_id won’t be NULL, but lpuart_soc_data Is NULL.
In linux-next we have:
static const struct of_device_id lpuart_dt_ids[] = {
{ .compatible = "fsl,vf610-lpuart", .data = &vf_data, },
{ .compatible = "fsl,ls1021a-lpuart", .data = &ls1021a_data, },
{ .compatible = "fsl,ls1028a-lpuart", .data = &ls1028a_data, },
{ .compatible = "fsl,imx7ulp-lpuart", .data = &imx7ulp_data, },
{ .compatible = "fsl,imx8qxp-lpuart", .data = &imx8qxp_data, },
{ /* sentinel */ }
};
All compatible entries have a .data field populated.
How sdata can be NULL?
On Mon, Apr 26, 2021 at 11:39:03AM +0000, Sherry Sun wrote:
> Hi Greg,
>
> > -----Original Message-----
> > From: Greg KH <[email protected]>
> > Sent: 2021年4月26日 16:09
> > To: Sherry Sun <[email protected]>
> > Cc: [email protected]; [email protected]; linux-
> > [email protected]; dl-linux-imx <[email protected]>
> > Subject: Re: [PATCH 2/2] tty: serial: fsl_lpuart: fix the potential bug of
> > dereference null return value
> >
> > On Mon, Apr 26, 2021 at 03:49:35PM +0800, Sherry Sun wrote:
> > > This issue is reported by Coverity Check.
> > > In lpuart_probe, return value of function which returns null is
> > > dereferenced without checking.
> > >
> > > Signed-off-by: Sherry Sun <[email protected]>
> > > ---
> > > drivers/tty/serial/fsl_lpuart.c | 3 +++
> > > 1 file changed, 3 insertions(+)
> > >
> > > diff --git a/drivers/tty/serial/fsl_lpuart.c
> > > b/drivers/tty/serial/fsl_lpuart.c index 777d54b593f8..c95e71fd2ca0
> > > 100644
> > > --- a/drivers/tty/serial/fsl_lpuart.c
> > > +++ b/drivers/tty/serial/fsl_lpuart.c
> > > @@ -2589,6 +2589,9 @@ static int lpuart_probe(struct platform_device
> > *pdev)
> > > struct resource *res;
> > > int ret;
> > >
> > > + if (!sdata)
> > > + return -ENODEV;
> >
> > How can sdata be NULL?
>
> Is it possible that a case forgot to set sdata? Then the value will be NULL, such as { .compatible = "fsl,imx8qxp-lpuart", }.
If a case forgets to set that somehow, then the driver will never work
with that kernel change, so someone better not submit that update :)
No need to check for something that is impossible to hit.
thanks,
greg k-h
On Mon, Apr 26, 2021 at 11:51:39AM +0000, Sherry Sun wrote:
> Hi Greg,
>
> > > > > drivers/tty/serial/fsl_lpuart.c | 3 +++
> > > > > 1 file changed, 3 insertions(+)
> > > > >
> > > > > diff --git a/drivers/tty/serial/fsl_lpuart.c
> > > > > b/drivers/tty/serial/fsl_lpuart.c index 794035041744..777d54b593f8
> > > > > 100644
> > > > > --- a/drivers/tty/serial/fsl_lpuart.c
> > > > > +++ b/drivers/tty/serial/fsl_lpuart.c
> > > > > @@ -2414,6 +2414,9 @@ lpuart32_console_get_options(struct
> > > > > lpuart_port *sport, int *baud,
> > > > >
> > > > > bd = lpuart32_read(&sport->port, UARTBAUD);
> > > > > bd &= UARTBAUD_SBR_MASK;
> > > > > + if (!bd)
> > > > > + return;
> > > >
> > > > How can this ever happen?
> > > >
> > > > Not to say this is a bad check, but it feels like this can't really
> > > > happen in real life, what code patch could create this result?
> > > >
> > > > And have you tested this on real hardware?
> > > >
> > >
> > > Thanks for the reviewing, yes, I have tested the patchset on the real
> > hardware.
> > >
> > > Seems the coverity check is static scan, so cannot judge if UARTBAUD
> > Register will be zero.
> > > I just found below statement in the uart reference manual: "When SBR is 1
> > - 8191, the baud rate equals "baud clock / ((OSR+1) ? SBR)"."
> > > Since I am not familiar with uart, do you mean that the value of UARTBAUD
> > Register will never be zero, so this case will not happen in real word?
> >
> > Given that this never has happened with hardware for such an old device,
> > perhaps it is impossible. But it would be good to check.
> >
> > > If yes, I will drop this patch.
> >
> > Handling "bad data" from hardware is never a bad idea, so I don't
> > necessarily want to drop this patch, I just want to try to figure out if this is a
> > "incase the hardware is broken/malicious" type of change, vs.
> > a "this bug we are seeing in real hardware" type of change.
> >
>
> Yes, you are right, the probability of hardware happen in this case is really low. But we cannot guarantee that it will never happen.
> So will this check here be accepted? Thanks!
Please resubmit it with a better changelog description summarizing the
discussion here to make it more obvious why this change is needed.
thanks,
greg k-h
Hi Greg,
> > Hi Greg,
> >
> > > -----Original Message-----
> > > From: Greg KH <[email protected]>
> > > Sent: 2021年4月26日 16:09
> > > To: Sherry Sun <[email protected]>
> > > Cc: [email protected]; [email protected]; linux-
> > > [email protected]; dl-linux-imx <[email protected]>
> > > Subject: Re: [PATCH 2/2] tty: serial: fsl_lpuart: fix the potential
> > > bug of dereference null return value
> > >
> > > On Mon, Apr 26, 2021 at 03:49:35PM +0800, Sherry Sun wrote:
> > > > This issue is reported by Coverity Check.
> > > > In lpuart_probe, return value of function which returns null is
> > > > dereferenced without checking.
> > > >
> > > > Signed-off-by: Sherry Sun <[email protected]>
> > > > ---
> > > > drivers/tty/serial/fsl_lpuart.c | 3 +++
> > > > 1 file changed, 3 insertions(+)
> > > >
> > > > diff --git a/drivers/tty/serial/fsl_lpuart.c
> > > > b/drivers/tty/serial/fsl_lpuart.c index 777d54b593f8..c95e71fd2ca0
> > > > 100644
> > > > --- a/drivers/tty/serial/fsl_lpuart.c
> > > > +++ b/drivers/tty/serial/fsl_lpuart.c
> > > > @@ -2589,6 +2589,9 @@ static int lpuart_probe(struct
> > > > platform_device
> > > *pdev)
> > > > struct resource *res;
> > > > int ret;
> > > >
> > > > + if (!sdata)
> > > > + return -ENODEV;
> > >
> > > How can sdata be NULL?
> >
> > Is it possible that a case forgot to set sdata? Then the value will be NULL,
> such as { .compatible = "fsl,imx8qxp-lpuart", }.
>
> If a case forgets to set that somehow, then the driver will never work with
> that kernel change, so someone better not submit that update :)
>
> No need to check for something that is impossible to hit.
Okay, got it, will drop this patch, thanks!
Best regards
Sherry
>
> thanks,
>
> greg k-h
Hi Fabio,
>
> > Thanks for your reply.
> > I guess you mean the of_match_table will not be NULL since it contains
> compatible, right?
> > But for the lpuart data -- struct lpuart_soc_data, won’t it meet the NULL
> case? such as { .compatible = "fsl,imx8qxp-lpuart", }.
> > Here of_device_id won’t be NULL, but lpuart_soc_data Is NULL.
>
> In linux-next we have:
>
> static const struct of_device_id lpuart_dt_ids[] = { { .compatible = "fsl,vf610-
> lpuart", .data = &vf_data, }, { .compatible = "fsl,ls1021a-lpuart", .data =
> &ls1021a_data, }, { .compatible = "fsl,ls1028a-lpuart", .data =
> &ls1028a_data, }, { .compatible = "fsl,imx7ulp-lpuart", .data =
> &imx7ulp_data, }, { .compatible = "fsl,imx8qxp-lpuart", .data =
> &imx8qxp_data, }, { /* sentinel */ } };
>
> All compatible entries have a .data field populated.
>
> How sdata can be NULL?
You are right, seems it is impossible for us to hit this NULL. Will drop this patch.
Best regards
Sherry
Hi Greg,
> > > > >
> > > >
> > > > Thanks for the reviewing, yes, I have tested the patchset on the
> > > > real
> > > hardware.
> > > >
> > > > Seems the coverity check is static scan, so cannot judge if
> > > > UARTBAUD
> > > Register will be zero.
> > > > I just found below statement in the uart reference manual: "When
> > > > SBR is 1
> > > - 8191, the baud rate equals "baud clock / ((OSR+1) ? SBR)"."
> > > > Since I am not familiar with uart, do you mean that the value of
> > > > UARTBAUD
> > > Register will never be zero, so this case will not happen in real word?
> > >
> > > Given that this never has happened with hardware for such an old
> > > device, perhaps it is impossible. But it would be good to check.
> > >
> > > > If yes, I will drop this patch.
> > >
> > > Handling "bad data" from hardware is never a bad idea, so I don't
> > > necessarily want to drop this patch, I just want to try to figure
> > > out if this is a "incase the hardware is broken/malicious" type of change,
> vs.
> > > a "this bug we are seeing in real hardware" type of change.
> > >
> >
> > Yes, you are right, the probability of hardware happen in this case is really
> low. But we cannot guarantee that it will never happen.
> > So will this check here be accepted? Thanks!
>
> Please resubmit it with a better changelog description summarizing the
> discussion here to make it more obvious why this change is needed.
>
Sure, will send a V2 patch with a better commit description. Thanks!
Best regards
Sherry
> thanks,
>
> greg k-h