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

["Harris, James R" <james.r.harris@intel.com>]

On 10/14/19, 4:18 AM, "David Marchand" <david.marchand@redhat.com> wrote:

    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?

Hi David,

I don't think this issue is common at all.  I don't have any known cases in mind - it was only found via code inspection.
    
    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] ?
    
You're right.  It needs to save mp_rep->fds[0] into a local variable before we free that array.  That would be a good follow-up patch!

-Jim

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