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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Aug 1, 2022 at 3:47 PM Dan Carpenter <dan.carpenter@xxxxxxxxxx> 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@xxxxxxxxxx>
> ---
>  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
>



[Index of Archives]     [Kernel Development]     [Kernel Announce]     [Kernel Newbies]     [Linux Networking Development]     [Share Photos]     [IDE]     [Security]     [Git]     [Netfilter]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Device Mapper]

  Powered by Linux