Re: [dpdk-dev] [PATCH] vfio: free mp_reply msgs in failure cases

[David Marchand <david.marchand@redhat.com>]

On Fri, Aug 16, 2019 at 9:19 PM Jim Harris <james.r.harris@intel.com> wrote:
>
> The code checks both rte_mp_request_sync() return
> code and that the number of messages in the reply
> equals 1.  If rte_mp_request_sync() succeeds but
> there was more than one message, those messages
> would get leaked.
>
> Found via code review by Anatoly Burakov of patches
> that used the vhost code as a template for using
> rte_mp_request_sync().

The patch looks fine, I just want to make sure its title reflect what it fixes.
Can you give some insights of how common this issue is? If there are
known cases where it happens?


I might have spotted another issue (could be worth a followup patch
later if confirmed), please see below.

>
> Signed-off-by: Jim Harris <james.r.harris@intel.com>
> ---
>  lib/librte_eal/linux/eal/eal_vfio.c |   16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/lib/librte_eal/linux/eal/eal_vfio.c b/lib/librte_eal/linux/eal/eal_vfio.c
> index 501c74f23..d9541b122 100644
> --- a/lib/librte_eal/linux/eal/eal_vfio.c
> +++ b/lib/librte_eal/linux/eal/eal_vfio.c

[snip]

> @@ -1021,7 +1021,7 @@ int
>  vfio_get_default_container_fd(void)
>  {
>         struct rte_mp_msg mp_req, *mp_rep;
> -       struct rte_mp_reply mp_reply;
> +       struct rte_mp_reply mp_reply = {0};
>         struct timespec ts = {.tv_sec = 5, .tv_nsec = 0};
>         struct vfio_mp_param *p = (struct vfio_mp_param *)mp_req.param;
>
> @@ -1049,9 +1049,9 @@ vfio_get_default_container_fd(void)
>                         free(mp_reply.msgs);
>                         return mp_rep->fds[0];

Do we have a use after free on mp_rep which points to &mp_reply.msgs[0] ?


>                 }
> -               free(mp_reply.msgs);
>         }
>
> +       free(mp_reply.msgs);
>         RTE_LOG(ERR, EAL, "  cannot request default container fd\n");
>         return -1;
>  }



-- 
David Marchand