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
>
