[dpdk-dev,1/8] vhost: add security model documentation to vhost_user.c
Checks
Commit Message
Input validation is not applied consistently in vhost_user.c. This
suggests that not everyone has the same security model in mind when
working on the code.
Make the security model explicit so that everyone can understand and
follow the same model when modifying the code.
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
lib/librte_vhost/vhost_user.c | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
Comments
> Input validation is not applied consistently in vhost_user.c. This suggests that
> not everyone has the same security model in mind when working on the
> code.
>
> Make the security model explicit so that everyone can understand and follow
> the same model when modifying the code.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
> lib/librte_vhost/vhost_user.c | 17 +++++++++++++++++
> 1 file changed, 17 insertions(+)
>
> diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
<...>
This is a useful comment but I don't know if it makes sense to include it in the vhost_user.c file.
Particularly at the top where it looks like a general descriptive comment for the file.
It would probably be better in the vhost-user section of the programmer's guide:
http://dpdk.org/doc/guides/prog_guide/vhost_lib.html
Also, the subject line shouldn't include an underscore.
Marko
<...>
> > This is a useful comment but I don't know if it makes sense to include it in
> the vhost_user.c file.
> >
> > Particularly at the top where it looks like a general descriptive comment for
> the file.
> >
> > It would probably be better in the vhost-user section of the programmer's
> guide:
> >
> > http://dpdk.org/doc/guides/prog_guide/vhost_lib.html
>
> That is the public API documentation for users of the library. They don't
> parse VhostUserMsg so I'm not sure the comment would be relevant there.
>
> > Also, the subject line shouldn't include an underscore.
>
> Do you mean the "vhost_user.c" filename in the subject line?
>
> Please explain why this isn't allowed.
>
> Stefan
You can run this command to check the git log.
./devtools/check-git-log.sh -1
Running git check log on HEAD~1: 34962
======================================
Wrong headline format:
vhost: add security model documentation to vhost_user.c
> -----Original Message-----
> From: Stefan Hajnoczi [mailto:stefanha@redhat.com]
> Sent: Tuesday, February 6, 2018 2:23 PM
> To: Kovacevic, Marko <marko.kovacevic@intel.com>
> Cc: dev@dpdk.org; Maxime Coquelin <maxime.coquelin@redhat.com>; Yuanhan
> Liu <yliu@fridaylinux.org>; Mcnamara, John <john.mcnamara@intel.com>
> Subject: Re: [dpdk-dev] [PATCH 1/8] vhost: add security model
> documentation to vhost_user.c
>
> On Tue, Feb 06, 2018 at 01:26:13PM +0000, Kovacevic, Marko wrote:
> > > Input validation is not applied consistently in vhost_user.c. This
> > > suggests that not everyone has the same security model in mind when
> > > working on the code.
> > >
> > > Make the security model explicit so that everyone can understand and
> > > follow the same model when modifying the code.
> > >
> > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > > ---
> > > lib/librte_vhost/vhost_user.c | 17 +++++++++++++++++
> > > 1 file changed, 17 insertions(+)
> > >
> > > diff --git a/lib/librte_vhost/vhost_user.c
> > > b/lib/librte_vhost/vhost_user.c
> >
> > <...>
> >
> > This is a useful comment but I don't know if it makes sense to include
> it in the vhost_user.c file.
> >
> > Particularly at the top where it looks like a general descriptive
> comment for the file.
> >
> > It would probably be better in the vhost-user section of the
> programmer's guide:
> >
> > http://dpdk.org/doc/guides/prog_guide/vhost_lib.html
>
> That is the public API documentation for users of the library. They don't
> parse VhostUserMsg so I'm not sure the comment would be relevant there.
Hi,
If it is public API documentation then it should probably be in a .h file
in doxygen format.
I'm in favour of the information being added, and thanks for that, just not
in that position in that file.
John
Hi John,
On 02/07/2018 05:10 PM, Mcnamara, John wrote:
>
>
>> -----Original Message-----
>> From: Stefan Hajnoczi [mailto:stefanha@redhat.com]
>> Sent: Tuesday, February 6, 2018 2:23 PM
>> To: Kovacevic, Marko <marko.kovacevic@intel.com>
>> Cc: dev@dpdk.org; Maxime Coquelin <maxime.coquelin@redhat.com>; Yuanhan
>> Liu <yliu@fridaylinux.org>; Mcnamara, John <john.mcnamara@intel.com>
>> Subject: Re: [dpdk-dev] [PATCH 1/8] vhost: add security model
>> documentation to vhost_user.c
>>
>> On Tue, Feb 06, 2018 at 01:26:13PM +0000, Kovacevic, Marko wrote:
>>>> Input validation is not applied consistently in vhost_user.c. This
>>>> suggests that not everyone has the same security model in mind when
>>>> working on the code.
>>>>
>>>> Make the security model explicit so that everyone can understand and
>>>> follow the same model when modifying the code.
>>>>
>>>> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>>>> ---
>>>> lib/librte_vhost/vhost_user.c | 17 +++++++++++++++++
>>>> 1 file changed, 17 insertions(+)
>>>>
>>>> diff --git a/lib/librte_vhost/vhost_user.c
>>>> b/lib/librte_vhost/vhost_user.c
>>>
>>> <...>
>>>
>>> This is a useful comment but I don't know if it makes sense to include
>> it in the vhost_user.c file.
>>>
>>> Particularly at the top where it looks like a general descriptive
>> comment for the file.
>>>
>>> It would probably be better in the vhost-user section of the
>> programmer's guide:
>>>
>>> http://dpdk.org/doc/guides/prog_guide/vhost_lib.html
>>
>> That is the public API documentation for users of the library. They don't
>> parse VhostUserMsg so I'm not sure the comment would be relevant there.
>
> Hi,
>
> If it is public API documentation then it should probably be in a .h file
> in doxygen format.
>
> I'm in favour of the information being added, and thanks for that, just not
> in that position in that file.
This is not part of the API but purely vhost-user lib internals,
so I think this is the right place for this comment.
It is more likely to be seen by the developer here than in a separate
file.
Cheers,
Maxime
> John
>
> -----Original Message-----
> From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com]
> Sent: Wednesday, February 7, 2018 4:18 PM
> To: Mcnamara, John <john.mcnamara@intel.com>; Stefan Hajnoczi
> <stefanha@redhat.com>; Kovacevic, Marko <marko.kovacevic@intel.com>
> Cc: dev@dpdk.org; Yuanhan Liu <yliu@fridaylinux.org>
> Subject: Re: [dpdk-dev] [PATCH 1/8] vhost: add security model
> documentation to vhost_user.c
>
> Hi John,
>
> On 02/07/2018 05:10 PM, Mcnamara, John wrote:
> >
> >
> >> -----Original Message-----
> >> From: Stefan Hajnoczi [mailto:stefanha@redhat.com]
> >> Sent: Tuesday, February 6, 2018 2:23 PM
> >> To: Kovacevic, Marko <marko.kovacevic@intel.com>
> >> Cc: dev@dpdk.org; Maxime Coquelin <maxime.coquelin@redhat.com>;
> >> Yuanhan Liu <yliu@fridaylinux.org>; Mcnamara, John
> >> <john.mcnamara@intel.com>
> >> Subject: Re: [dpdk-dev] [PATCH 1/8] vhost: add security model
> >> documentation to vhost_user.c
> >>
> >> On Tue, Feb 06, 2018 at 01:26:13PM +0000, Kovacevic, Marko wrote:
> >>>> Input validation is not applied consistently in vhost_user.c. This
> >>>> suggests that not everyone has the same security model in mind when
> >>>> working on the code.
> >>>>
> >>>> Make the security model explicit so that everyone can understand
> >>>> and follow the same model when modifying the code.
> >>>>
> >>>> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> >>>> ---
> >>>> lib/librte_vhost/vhost_user.c | 17 +++++++++++++++++
> >>>> 1 file changed, 17 insertions(+)
> >>>>
> >>>> diff --git a/lib/librte_vhost/vhost_user.c
> >>>> b/lib/librte_vhost/vhost_user.c
> >>>
> >>> <...>
> >>>
> >>> This is a useful comment but I don't know if it makes sense to
> >>> include
> >> it in the vhost_user.c file.
> >>>
> >>> Particularly at the top where it looks like a general descriptive
> >> comment for the file.
> >>>
> >>> It would probably be better in the vhost-user section of the
> >> programmer's guide:
> >>>
> >>> http://dpdk.org/doc/guides/prog_guide/vhost_lib.html
> >>
> >> That is the public API documentation for users of the library. They
> >> don't parse VhostUserMsg so I'm not sure the comment would be relevant
> there.
> >
> > Hi,
> >
> > If it is public API documentation then it should probably be in a .h
> > file in doxygen format.
> >
> > I'm in favour of the information being added, and thanks for that,
> > just not in that position in that file.
>
> This is not part of the API but purely vhost-user lib internals, so I
> think this is the right place for this comment.
>
> It is more likely to be seen by the developer here than in a separate
> file.
Ok. In that case:
Acked-by: John McNamara <john.mcnamara@intel.com>
@@ -2,6 +2,23 @@
* Copyright(c) 2010-2016 Intel Corporation
*/
+/* Security model
+ * --------------
+ * The vhost-user protocol connection is an external interface, so it must be
+ * robust against invalid inputs.
+ *
+ * This is important because the vhost-user master is only one step removed
+ * from the guest. Malicious guests that have escaped will then launch further
+ * attacks from the vhost-user master.
+ *
+ * Even in deployments where guests are trusted, a bug in the vhost-user master
+ * can still cause invalid messages to be sent. Such messages must not
+ * compromise the stability of the DPDK application by causing crashes, memory
+ * corruption, or other problematic behavior.
+ *
+ * Do not assume received VhostUserMsg fields contain sensible values!
+ */
+
#include <stdint.h>
#include <stdio.h>
#include <stdlib.h>