[PATCH 1/2] NTB: EPF: Fix error code in epf_ntb_bind()

[PATCH 1/2] NTB: EPF: Fix error code in epf_ntb_bind()

[Dan Carpenter]

Return an error code if pci_register_driver() fails.  Don't return
success.

Fixes: da51fd247424 ("NTB: EPF: support NTB transfer between PCI RC and EP connection")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
 drivers/pci/endpoint/functions/pci-epf-vntb.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/endpoint/functions/pci-epf-vntb.c b/drivers/pci/endpoint/functions/pci-epf-vntb.c
index 111568089d45..cf338f3cf348 100644
--- a/drivers/pci/endpoint/functions/pci-epf-vntb.c
+++ b/drivers/pci/endpoint/functions/pci-epf-vntb.c
@@ -1312,7 +1312,8 @@ static int epf_ntb_bind(struct pci_epf *epf)
 
 	epf_set_drvdata(epf, ntb);
 
-	if (pci_register_driver(&vntb_pci_driver)) {
+	ret = pci_register_driver(&vntb_pci_driver);
+	if (ret) {
 		dev_err(dev, "failure register vntb pci driver\n");
 		goto err_bar_alloc;
 	}
-- 
2.35.1

[PATCH 2/2] NTB: EPF: Tidy up some bounds checks

[Dan Carpenter]

This sscanf() is reading from the filename which was set by the kernel
so it should be trust worthy.  Although the data is likely trust worthy
there is some bounds checking but unfortunately, it is not complete or
consistent.  Additionally, the Smatch static checker marks everything
that comes from sscanf() as tainted and so Smatch complains that this
code can lead to an out of bounds issue.  Let's clean things up and make
Smatch happy.

The first problem is that there is no bounds checking in the _show()
functions.  The _store() and _show() functions are very similar so make
the bounds checking the same in both.

The second issue is that if "win_no" is zero it leads to an array
underflow so add an if (win_no <= 0) check for that.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
 drivers/pci/endpoint/functions/pci-epf-vntb.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/endpoint/functions/pci-epf-vntb.c b/drivers/pci/endpoint/functions/pci-epf-vntb.c
index cf338f3cf348..a7fe86f43739 100644
--- a/drivers/pci/endpoint/functions/pci-epf-vntb.c
+++ b/drivers/pci/endpoint/functions/pci-epf-vntb.c
@@ -831,9 +831,16 @@ static ssize_t epf_ntb_##_name##_show(struct config_item *item,		\
 {									\
 	struct config_group *group = to_config_group(item);		\
 	struct epf_ntb *ntb = to_epf_ntb(group);			\
+	struct device *dev = &ntb->epf->dev;				\
 	int win_no;							\
 									\
-	sscanf(#_name, "mw%d", &win_no);				\
+	if (sscanf(#_name, "mw%d", &win_no) != 1)			\
+		return -EINVAL;						\
+									\
+	if (win_no <= 0 || win_no > ntb->num_mws) {			\
+		dev_err(dev, "Invalid num_nws: %d value\n", ntb->num_mws); \
+		return -EINVAL;						\
+	}								\
 									\
 	return sprintf(page, "%lld\n", ntb->mws_size[win_no - 1]);	\
 }
@@ -856,7 +863,7 @@ static ssize_t epf_ntb_##_name##_store(struct config_item *item,	\
 	if (sscanf(#_name, "mw%d", &win_no) != 1)			\
 		return -EINVAL;						\
 									\
-	if (ntb->num_mws < win_no) {					\
+	if (win_no <= 0 || win_no > ntb->num_mws) {			\
 		dev_err(dev, "Invalid num_nws: %d value\n", ntb->num_mws); \
 		return -EINVAL;						\
 	}								\
-- 
2.35.1

Re: [PATCH 2/2] NTB: EPF: Tidy up some bounds checks

[Souptick Joarder]

On Mon, Aug 1, 2022 at 3:47 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> This sscanf() is reading from the filename which was set by the kernel
> so it should be trust worthy.  Although the data is likely trust worthy
> there is some bounds checking but unfortunately, it is not complete or
> consistent.  Additionally, the Smatch static checker marks everything
> that comes from sscanf() as tainted and so Smatch complains that this
> code can lead to an out of bounds issue.  Let's clean things up and make
> Smatch happy.
>
> The first problem is that there is no bounds checking in the _show()
> functions.  The _store() and _show() functions are very similar so make
> the bounds checking the same in both.
>
> The second issue is that if "win_no" is zero it leads to an array
> underflow so add an if (win_no <= 0) check for that.
>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
>  drivers/pci/endpoint/functions/pci-epf-vntb.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/endpoint/functions/pci-epf-vntb.c b/drivers/pci/endpoint/functions/pci-epf-vntb.c
> index cf338f3cf348..a7fe86f43739 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-vntb.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-vntb.c
> @@ -831,9 +831,16 @@ static ssize_t epf_ntb_##_name##_show(struct config_item *item,            \
>  {                                                                      \
>         struct config_group *group = to_config_group(item);             \
>         struct epf_ntb *ntb = to_epf_ntb(group);                        \
> +       struct device *dev = &ntb->epf->dev;                            \
>         int win_no;                                                     \
>                                                                         \
> -       sscanf(#_name, "mw%d", &win_no);                                \
> +       if (sscanf(#_name, "mw%d", &win_no) != 1)                       \
> +               return -EINVAL;                                         \
> +                                                                       \
> +       if (win_no <= 0 || win_no > ntb->num_mws) {                     \
> +               dev_err(dev, "Invalid num_nws: %d value\n", ntb->num_mws); \
> +               return -EINVAL;                                         \
> +       }                                                               \
>                                                                         \
>         return sprintf(page, "%lld\n", ntb->mws_size[win_no - 1]);      \
>  }
> @@ -856,7 +863,7 @@ static ssize_t epf_ntb_##_name##_store(struct config_item *item,    \
>         if (sscanf(#_name, "mw%d", &win_no) != 1)                       \
>                 return -EINVAL;                                         \
>                                                                         \
> -       if (ntb->num_mws < win_no) {                                    \
> +       if (win_no <= 0 || win_no > ntb->num_mws) {                     \

This might change the existing logic. will it be ?
if (win_no <= 0 || win_no >= ntb->num_mws) {


>                 dev_err(dev, "Invalid num_nws: %d value\n", ntb->num_mws); \
>                 return -EINVAL;                                         \
>         }                                                               \
> --
> 2.35.1
>

Re: [PATCH 2/2] NTB: EPF: Tidy up some bounds checks

[Dan Carpenter]

On Tue, Aug 02, 2022 at 04:19:33PM +0530, Souptick Joarder wrote:
> On Mon, Aug 1, 2022 at 3:47 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> >
> > This sscanf() is reading from the filename which was set by the kernel
> > so it should be trust worthy.  Although the data is likely trust worthy
> > there is some bounds checking but unfortunately, it is not complete or
> > consistent.  Additionally, the Smatch static checker marks everything
> > that comes from sscanf() as tainted and so Smatch complains that this
> > code can lead to an out of bounds issue.  Let's clean things up and make
> > Smatch happy.
> >
> > The first problem is that there is no bounds checking in the _show()
> > functions.  The _store() and _show() functions are very similar so make
> > the bounds checking the same in both.
> >
> > The second issue is that if "win_no" is zero it leads to an array
> > underflow so add an if (win_no <= 0) check for that.
> >
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > ---
> >  drivers/pci/endpoint/functions/pci-epf-vntb.c | 11 +++++++++--
> >  1 file changed, 9 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/pci/endpoint/functions/pci-epf-vntb.c b/drivers/pci/endpoint/functions/pci-epf-vntb.c
> > index cf338f3cf348..a7fe86f43739 100644
> > --- a/drivers/pci/endpoint/functions/pci-epf-vntb.c
> > +++ b/drivers/pci/endpoint/functions/pci-epf-vntb.c
> > @@ -831,9 +831,16 @@ static ssize_t epf_ntb_##_name##_show(struct config_item *item,            \
> >  {                                                                      \
> >         struct config_group *group = to_config_group(item);             \
> >         struct epf_ntb *ntb = to_epf_ntb(group);                        \
> > +       struct device *dev = &ntb->epf->dev;                            \
> >         int win_no;                                                     \
> >                                                                         \
> > -       sscanf(#_name, "mw%d", &win_no);                                \
> > +       if (sscanf(#_name, "mw%d", &win_no) != 1)                       \
> > +               return -EINVAL;                                         \
> > +                                                                       \
> > +       if (win_no <= 0 || win_no > ntb->num_mws) {                     \
> > +               dev_err(dev, "Invalid num_nws: %d value\n", ntb->num_mws); \
> > +               return -EINVAL;                                         \
> > +       }                                                               \
> >                                                                         \
> >         return sprintf(page, "%lld\n", ntb->mws_size[win_no - 1]);      \
> >  }
> > @@ -856,7 +863,7 @@ static ssize_t epf_ntb_##_name##_store(struct config_item *item,    \
> >         if (sscanf(#_name, "mw%d", &win_no) != 1)                       \
> >                 return -EINVAL;                                         \
> >                                                                         \
> > -       if (ntb->num_mws < win_no) {                                    \
> > +       if (win_no <= 0 || win_no > ntb->num_mws) {                     \
> 
> This might change the existing logic. will it be ?
> if (win_no <= 0 || win_no >= ntb->num_mws) {

No, it doesn't change the exiting logic with regards to the upper
bounds. if (foo > bar) is the same as if (bar < foo).  It just adds a
lower bounds check.

Normally, the variable part of the condition is on the right.  Check
patch enforces that rule where it can.  It's the same in English, the
variable comes first.  "If twelve foot is greater than the height of the
tree then blah blah" vs "if the tree is less than twelve foot blah
blah".

regards,
dan carpenter

Re: [PATCH 2/2] NTB: EPF: Tidy up some bounds checks

[Dan Carpenter]

On Tue, Aug 02, 2022 at 02:01:33PM +0300, Dan Carpenter wrote:
> It's the same in English, the variable comes first.  "If twelve foot
> is greater than the height of the tree then blah blah" vs "if the tree
> is less than twelve foot blah blah".

Hehe...  I had a brief momement of panic where I worried that these
statements might not be equivalent.  The backwards if is so confusing.

regards,
dan carpenter

Re: [PATCH 2/2] NTB: EPF: Tidy up some bounds checks

[Souptick Joarder]

On Tue, Aug 2, 2022 at 4:50 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> On Tue, Aug 02, 2022 at 02:01:33PM +0300, Dan Carpenter wrote:
> > It's the same in English, the variable comes first.  "If twelve foot
> > is greater than the height of the tree then blah blah" vs "if the tree
> > is less than twelve foot blah blah".
>
> Hehe...  I had a brief momement of panic where I worried that these
> statements might not be equivalent.  The backwards if is so confusing.

hehe.. Sorry I read the condition wrong with an example value at first.

Acked-by: Souptick Joarder (HPE) <jrdr.linux@gmail.com>

>
> regards,
> dan carpenter

Re: [PATCH 1/2] NTB: EPF: Fix error code in epf_ntb_bind()

[Souptick Joarder]

On Mon, Aug 1, 2022 at 3:45 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> Return an error code if pci_register_driver() fails.  Don't return
> success.
>
> Fixes: da51fd247424 ("NTB: EPF: support NTB transfer between PCI RC and EP connection")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

Acked-by: Souptick Joarder (HPE) <jrdr.linux@gmail.com>

> ---
>  drivers/pci/endpoint/functions/pci-epf-vntb.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pci/endpoint/functions/pci-epf-vntb.c b/drivers/pci/endpoint/functions/pci-epf-vntb.c
> index 111568089d45..cf338f3cf348 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-vntb.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-vntb.c
> @@ -1312,7 +1312,8 @@ static int epf_ntb_bind(struct pci_epf *epf)
>
>         epf_set_drvdata(epf, ntb);
>
> -       if (pci_register_driver(&vntb_pci_driver)) {
> +       ret = pci_register_driver(&vntb_pci_driver);
> +       if (ret) {
>                 dev_err(dev, "failure register vntb pci driver\n");
>                 goto err_bar_alloc;
>         }
> --
> 2.35.1
>

Re: [PATCH 1/2] NTB: EPF: Fix error code in epf_ntb_bind()

[Jon Mason]

On Mon, Aug 01, 2022 at 01:15:25PM +0300, Dan Carpenter wrote:
> Return an error code if pci_register_driver() fails.  Don't return
> success.
> 
> Fixes: da51fd247424 ("NTB: EPF: support NTB transfer between PCI RC and EP connection")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
>  drivers/pci/endpoint/functions/pci-epf-vntb.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/endpoint/functions/pci-epf-vntb.c b/drivers/pci/endpoint/functions/pci-epf-vntb.c
> index 111568089d45..cf338f3cf348 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-vntb.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-vntb.c
> @@ -1312,7 +1312,8 @@ static int epf_ntb_bind(struct pci_epf *epf)
>  
>  	epf_set_drvdata(epf, ntb);
>  
> -	if (pci_register_driver(&vntb_pci_driver)) {
> +	ret = pci_register_driver(&vntb_pci_driver);
> +	if (ret) {
>  		dev_err(dev, "failure register vntb pci driver\n");
>  		goto err_bar_alloc;
>  	}
> -- 
> 2.35.1
> 

Sorry for the delay in response.  This series is in my
ntb branch and will be in my pull request for v5.20 which should be
going out later today.

Thanks,
Jon