[dpdk-dev,1/8] vhost: add security model documentation to vhost_user.c

Message ID 20180205121642.26428-2-stefanha@redhat.com
State Accepted, archived
Delegated to: Maxime Coquelin
Headers show

Checks

Context Check Description
ci/Intel-compilation success Compilation OK
ci/checkpatch success coding style OK

Commit Message

Stefan Hajnoczi Feb. 5, 2018, 12:16 p.m.
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

Kovacevic, Marko Feb. 6, 2018, 1:26 p.m. | #1
> 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
Kovacevic, Marko Feb. 6, 2018, 3:39 p.m. | #2
<...>

> > 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
Mcnamara, John Feb. 7, 2018, 4:10 p.m. | #3
> -----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
Maxime Coquelin Feb. 7, 2018, 4:18 p.m. | #4
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
>
Mcnamara, John Feb. 7, 2018, 5:23 p.m. | #5
> -----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>

Patch

diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
index 1dd1a61b6..a96afbe84 100644
--- a/lib/librte_vhost/vhost_user.c
+++ b/lib/librte_vhost/vhost_user.c
@@ -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>