vfio: free mp_reply msgs in failure cases
Checks
Commit Message
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().
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(-)
Comments
On 16-Aug-19 1:13 PM, Jim Harris 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().
>
> 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
> @@ -264,7 +264,7 @@ vfio_open_group_fd(int iommu_group_num)
> int vfio_group_fd;
> char filename[PATH_MAX];
> 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;
>
> @@ -320,9 +320,9 @@ vfio_open_group_fd(int iommu_group_num)
> RTE_LOG(ERR, EAL, " bad VFIO group fd\n");
> vfio_group_fd = 0;
> }
> - free(mp_reply.msgs);
> }
>
> + free(mp_reply.msgs);
That's not quite correct. This fixes the problem of missing free() when
nb_received mismatches, but this /adds/ a problem of doing an
unnecessary free() when rte_mp_request_sync() returns -1. Same for other
places, i believe.
> On Aug 20, 2019, at 6:13 AM, Burakov, Anatoly <anatoly.burakov@intel.com> wrote:
>
>> On 16-Aug-19 1:13 PM, Jim Harris 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().
>> 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
>> @@ -264,7 +264,7 @@ vfio_open_group_fd(int iommu_group_num)
>> int vfio_group_fd;
>> char filename[PATH_MAX];
>> 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;
>> @@ -320,9 +320,9 @@ vfio_open_group_fd(int iommu_group_num)
>> RTE_LOG(ERR, EAL, " bad VFIO group fd\n");
>> vfio_group_fd = 0;
>> }
>> - free(mp_reply.msgs);
>> }
>> + free(mp_reply.msgs);
>
> That's not quite correct. This fixes the problem of missing free() when nb_received mismatches, but this /adds/ a problem of doing an unnecessary free() when rte_mp_request_sync() returns -1. Same for other places, i believe.
This would just resolve to free(NULL) in the -1 case.
Jim
>
> --
> Thanks,
> Anatoly
On 20-Aug-19 2:16 PM, Harris, James R wrote:
>
>
>> On Aug 20, 2019, at 6:13 AM, Burakov, Anatoly <anatoly.burakov@intel.com> wrote:
>>
>>> On 16-Aug-19 1:13 PM, Jim Harris 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().
>>> 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
>>> @@ -264,7 +264,7 @@ vfio_open_group_fd(int iommu_group_num)
>>> int vfio_group_fd;
>>> char filename[PATH_MAX];
>>> 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;
>>> @@ -320,9 +320,9 @@ vfio_open_group_fd(int iommu_group_num)
>>> RTE_LOG(ERR, EAL, " bad VFIO group fd\n");
>>> vfio_group_fd = 0;
>>> }
>>> - free(mp_reply.msgs);
>>> }
>>> + free(mp_reply.msgs);
>>
>> That's not quite correct. This fixes the problem of missing free() when nb_received mismatches, but this /adds/ a problem of doing an unnecessary free() when rte_mp_request_sync() returns -1. Same for other places, i believe.
>
> This would just resolve to free(NULL) in the -1 case.
>
Ah, you're right! We did fix that bug :)
With that in mind,
Acked-by: Anatoly Burakov <anatoly.burakov@intel.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;
> }
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
On Mon, Oct 14, 2019 at 3:49 PM Harris, James R
<james.r.harris@intel.com> wrote:
> 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.
Anatoly, Jim,
Not really inspired for the title, what do you think of:
vfio: fix potential leak with multiprocess
Plus, it deserves a Fixes: line.
Fixes: 83a73c5fef66 ("vfio: use generic multi-process channel")
Cc: stable@dpdk.org
If you are okay with this, I will do the change when applying.
> On Oct 14, 2019, at 7:47 AM, David Marchand <david.marchand@redhat.com> wrote:
>
> On Mon, Oct 14, 2019 at 3:49 PM Harris, James R
> <james.r.harris@intel.com> wrote:
>> 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.
>
> Anatoly, Jim,
>
> Not really inspired for the title, what do you think of:
> vfio: fix potential leak with multiprocess
>
> Plus, it deserves a Fixes: line.
> Fixes: 83a73c5fef66 ("vfio: use generic multi-process channel")
> Cc: stable@dpdk.org
>
> If you are okay with this, I will do the change when applying.
I am ok with those changes. Thanks!
>
> --
> David Marchand
>
On Mon, Oct 14, 2019 at 4:47 PM David Marchand
<david.marchand@redhat.com> wrote:
>
> On Mon, Oct 14, 2019 at 3:49 PM Harris, James R
> <james.r.harris@intel.com> wrote:
> > 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.
>
> Anatoly, Jim,
>
> Not really inspired for the title, what do you think of:
> vfio: fix potential leak with multiprocess
>
> Plus, it deserves a Fixes: line.
> Fixes: 83a73c5fef66 ("vfio: use generic multi-process channel")
> Cc: stable@dpdk.org
>
> If you are okay with this, I will do the change when applying.
Applied, thanks.
--
David Marchand
@@ -264,7 +264,7 @@ vfio_open_group_fd(int iommu_group_num)
int vfio_group_fd;
char filename[PATH_MAX];
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;
@@ -320,9 +320,9 @@ vfio_open_group_fd(int iommu_group_num)
RTE_LOG(ERR, EAL, " bad VFIO group fd\n");
vfio_group_fd = 0;
}
- free(mp_reply.msgs);
}
+ free(mp_reply.msgs);
if (vfio_group_fd < 0)
RTE_LOG(ERR, EAL, " cannot request group fd\n");
return vfio_group_fd;
@@ -554,7 +554,7 @@ static int
vfio_sync_default_container(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;
int iommu_type_id;
@@ -584,8 +584,8 @@ vfio_sync_default_container(void)
p = (struct vfio_mp_param *)mp_rep->param;
if (p->result == SOCKET_OK)
iommu_type_id = p->iommu_type_id;
- free(mp_reply.msgs);
}
+ free(mp_reply.msgs);
if (iommu_type_id < 0) {
RTE_LOG(ERR, EAL, "Could not get IOMMU type for default container\n");
return -1;
@@ -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];
}
- free(mp_reply.msgs);
}
+ free(mp_reply.msgs);
RTE_LOG(ERR, EAL, " cannot request default container fd\n");
return -1;
}
@@ -1127,7 +1127,7 @@ rte_vfio_get_container_fd(void)
{
int ret, vfio_container_fd;
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;
@@ -1181,9 +1181,9 @@ rte_vfio_get_container_fd(void)
free(mp_reply.msgs);
return vfio_container_fd;
}
- free(mp_reply.msgs);
}
+ free(mp_reply.msgs);
RTE_LOG(ERR, EAL, " cannot request container fd\n");
return -1;
}