[dpdk-dev] thoughts on DPDK after a few days of reading sources

[dpdk-dev] thoughts on DPDK after a few days of reading sources

[Seth Arnold]

Hello,

The Ubuntu distribution is looking at supporting DPDK in the 'main'
component of the archive. As part of this process I spent a few days
reading the DPDK sources to gauge if we can support it or not.

I've taken some notes while reading the sources; I'm sharing them in the
hopes that it's useful: on the one hand my fresh eyes may spot things that
you've overlooked, on the other hand your familiarity with the code means
that you're better suited to judge what I've found.

Most of the code was very good; I am however concerned about the frequent
memory allocations that use simple integer arithmetic when deciding how
much to allocate without checking for integer overflows.

Here's the bug tracking the Main Inclusion Request:
https://bugs.launchpad.net/ubuntu/+source/dpdk/+bug/1492186

Here's the portions of the notes that I think may be most interesting
to DPDK devs. It's in rough priority order so feel free to stop reading
when you're bored:

===

Affects main codebase and should be investigated quickly:

- shellcheck reports extensive cases of forgotten quotes to prevent word
  splitting or globbing, potentially unused variables, error-prone printf
  formatting. The scripts that are going to be used at runtime should be
  fixed:
  - ./debian/dpdk-init
  - ./debian/dpdk.init
  - ./tools/setup.sh ? (Hard to tell)
- ./drivers/net/cxgbe/cxgbe_ethdev.c eth_cxgbe_dev_init() memory leak in
  out_free_adapter: that doesn't free adapter
- ./drivers/net/virtio/virtio_ethdev.c virtio_set_multiple_queues() calls
  virtio_send_command(), which performs:
  memcpy(vq->virtio_net_hdr_mz->addr, ctrl, sizeof(struct virtio_pmd_ctrl));
  This copies a potentially huge amount of uninitialized data into ->addr
  because the struct virtio_pmd_ctrl ctrl was not zeroed before being
  passed. How much of this data leaves the system? Does this require a
  CVE?
- ./lib/librte_eal/common/rte_malloc.c memory allocation routines don't
  check for integer overflow errors:
  - rte_calloc_socket()
  - rte_calloc()
- ./lib/librte_vhost/vhost_user/virtio-net-user.c user_set_mem_table()
  doesn't perform integer overflow checks before calling calloc()
- ./lib/librte_vhost/vhost_cuse/virtio-net-cdev.c cuse_set_mem_table()
  doesn't perform integer overflow checks before calling calloc()
- ./lib/librte_eal/linuxapp/eal/eal_xen_memory.c rte_xen_dom0_memory_init()
  If vma_addr = xen_get_virtual_area(&vma_len, RTE_PGSIZE_2M); fails,
  vma_len is reset to RTE_PGSIZE_2M instead of seginfo[memseg_idx].size --
  is this a bug?
- ./lib/librte_eal/linuxapp/eal/eal_memory.c create_shared_memory()
  creates the hugetlb file with mode 0666 rather than 0600 -- is this a
  bug? Does this require a CVE?
- ./lib/librte_eal/common/include/arch/ppc_64/rte_cpuflags.h
  rte_cpu_get_features() leaks auxv_fd
- ./lib/librte_eal/common/include/arch/arm/rte_cpuflags_32.h
  rte_cpu_get_features() leaks auxv_fd
- ./lib/librte_eal/common/include/arch/arm/rte_cpuflags_64.h
  rte_cpu_get_features() leaks auxv_fd

Affects main codebase:

- Assorted false-positives and style issues reported by cppcheck
- Slightly dangerous convention of memcpy(dest, source, sizeof(source))
  is used extensively; while all the instances I investigated were
  correct, it's still more prone to mistakes under maintenance than
  memcpy(dest, source, sizeof(dest)).
- extensive use of *malloc() wrappers that perform multiplication to
  determine the size to allocate; all the cases I've seen used values that
  should be constrained by system configurations but the habit is
  dangerous compared to use of *calloc() wrappers that handle integer
  overflow safely.
- ./app/test/test_malloc.c test_reordered_free_per_lcore() has incorrect
  calls to is_memory_overlap() -- p2 is 16000 bytes long, not 1000 bytes
  long, and this size difference is not reflected in the calls.
- ./app/test/test_malloc.c test_realloc() leaks ptr1, may leak ptr9 via
  error path
- ./app/test/test_malloc.c test_realloc() the test with error "Unexpected
  - ptr4 != ptr3" doesn't feel like it tests an actual promise from the API
- ./lib/librte_pipeline/rte_pipeline.c rte_pipeline_table_create(),
  rte_pipeline_port_in_create(), rte_pipeline_port_out_create(),
  duplicate the array of function pointers via memcpy() rather than
  copying a pointer to static tables -- this may represent an easy way to
  save memory and improve cache hit ratios as well as potentially allow
  storing the tables in static memory rather than on the heap, reducing
  the value of these structs in potential exploits.
- ixgbe driver in the package is very different from the driver in the
  Linux kernel -- when bugs in one are found, who is in charge of copying
  the fixes between the two code bases?
- ./lib/librte_eal/linuxapp/kni/ethtool/ixgbe/ixgbe_ethtool.c
  ixgbe_get_strings() takes a buffer to write into but not the buffer
  length; sprintf() calls may overflow the buffer if it isn't large
  enough. It looks like ethtool_get_strings() may use fixed-size buffer
  but the amount of data that gets written into it is based on both static
  and dynamic data. Can this overflow the buffer?
- ./lib/librte_eal/linuxapp/eal/eal.c rte_eal_config_create() uses mode
  0660 rather than 0600 to create /home/username/.<something>_config
  files, which may be too open on distributions with "staff" or "users"
  default user groups rather than user-specific groups.

Affects examples but should still be investigated quickly:

- print_stats() in examples/l2fwd-ivshmem/host/host.c uses variable
  total_vm_packets_dropped without initialization

Affects examples:

- app_config_preproc() from ./examples/ip_pipeline/config_parse.c builds a
  string to execute with system(). While the inputs come from the command
  line arguments, they might have been supplied via a safe mechanism from
  untrusted users, rendering them unsafe here. The snprintf() error return
  is ignored. The access(app->config_file) check isn't as helpful as an
  actual error message if something does go wrong.
- main() from ./examples/kni/main.c ignores kni_alloc() return value
- main() from ./examples/kni/main.c may allow port to grow beyond
  RTE_MAX_ETHPORTS and then aborts rather than capping the upper end of
  the for() loop.
- ./examples/vhost/main.c txmbuf_clean_zcp():
        uint32_t used_idx = vq->last_used_idx & (vq->size - 1);
  Use of & rather than % for modular arithmetic -- are virtuqueue sizes
  guaranteed to be powers of two?
- Multiple places omits errno in the case the file open fails:
  - ./examples/ip_pipeline/config_parse.c app_config_save()
  - ./examples/ip_pipeline/pipeline/pipeline_common_fe.c app_run_file()
  - ./examples/ethtool/ethtool-app/ethapp.c pcmd_eeprom_callback()
  - other locations of this aren't mentioned.
  It's nearly impossible to solve issues without error reporting. Good
  error reporting saves admins time and money.

===

Thanks

Re: [dpdk-dev] thoughts on DPDK after a few days of reading sources

[Matthew Hall]

On Wed, Feb 10, 2016 at 07:05:40PM -0800, Seth Arnold wrote:
> - ixgbe driver in the package is very different from the driver in the
>   Linux kernel -- when bugs in one are found, who is in charge of copying
>   the fixes between the two code bases?

It's not the Linux driver. It's from BSD because DPDK uses mbufs. The 
MAINTAINERS file / Intel team refreshes it w/ shared code from their upstream 
BSD support.

Matthew.

Re: [dpdk-dev] thoughts on DPDK after a few days of reading sources

[Thomas Monjalon]

Hi,

2016-02-10 19:05, Seth Arnold:
> I've taken some notes while reading the sources; I'm sharing them in the
> hopes that it's useful: on the one hand my fresh eyes may spot things that
> you've overlooked, on the other hand your familiarity with the code means
> that you're better suited to judge what I've found.

Thanks for taking time and sharing, it's very valuable.

> - shellcheck reports extensive cases of forgotten quotes to prevent word
>   splitting or globbing, potentially unused variables, error-prone printf
>   formatting. The scripts that are going to be used at runtime should be
>   fixed:
>   - ./debian/dpdk-init
>   - ./debian/dpdk.init

These files are not in the tree. Should they?

> - ./drivers/net/cxgbe/cxgbe_ethdev.c eth_cxgbe_dev_init() memory leak in
>   out_free_adapter: that doesn't free adapter
> - ./drivers/net/virtio/virtio_ethdev.c virtio_set_multiple_queues() calls
>   virtio_send_command(), which performs:
>   memcpy(vq->virtio_net_hdr_mz->addr, ctrl, sizeof(struct virtio_pmd_ctrl));
>   This copies a potentially huge amount of uninitialized data into ->addr
>   because the struct virtio_pmd_ctrl ctrl was not zeroed before being
>   passed. How much of this data leaves the system? Does this require a
>   CVE?

We are not used to open a CVE.

[...]
>   It's nearly impossible to solve issues without error reporting. Good
>   error reporting saves admins time and money.

Until now, the errors were reported on the list and most often fixed quickly.
While I agree we need a more formal process (a bug tracker), I think we must
be noticed of new bugs on the mailing list.
Since nobody was against the bugzilla proposal, a deployment will be planned.
	http://dpdk.org/ml/archives/dev/2015-August/023012.html

Re: [dpdk-dev] thoughts on DPDK after a few days of reading sources

[Alejandro Lucero]

Hi Seth,

I do not know if you and Ubuntu know about the kernel VFIO no-iommu mode
which DPDK will use in the future (then getting rid of UIO drives).

This implies distributions enabling that kernel VFIO mode which is not
enable by default as it is a security issue.

It would be good to know which is the Ubuntu position regarding this issue
and if there are any date or plan for supporting this.

Thanks

On Thu, Feb 11, 2016 at 7:58 AM, Thomas Monjalon <thomas.monjalon@6wind.com>
wrote:

> Hi,
>
> 2016-02-10 19:05, Seth Arnold:
> > I've taken some notes while reading the sources; I'm sharing them in the
> > hopes that it's useful: on the one hand my fresh eyes may spot things
> that
> > you've overlooked, on the other hand your familiarity with the code means
> > that you're better suited to judge what I've found.
>
> Thanks for taking time and sharing, it's very valuable.
>
> > - shellcheck reports extensive cases of forgotten quotes to prevent word
> >   splitting or globbing, potentially unused variables, error-prone printf
> >   formatting. The scripts that are going to be used at runtime should be
> >   fixed:
> >   - ./debian/dpdk-init
> >   - ./debian/dpdk.init
>
> These files are not in the tree. Should they?
>
> > - ./drivers/net/cxgbe/cxgbe_ethdev.c eth_cxgbe_dev_init() memory leak in
> >   out_free_adapter: that doesn't free adapter
> > - ./drivers/net/virtio/virtio_ethdev.c virtio_set_multiple_queues() calls
> >   virtio_send_command(), which performs:
> >   memcpy(vq->virtio_net_hdr_mz->addr, ctrl, sizeof(struct
> virtio_pmd_ctrl));
> >   This copies a potentially huge amount of uninitialized data into ->addr
> >   because the struct virtio_pmd_ctrl ctrl was not zeroed before being
> >   passed. How much of this data leaves the system? Does this require a
> >   CVE?
>
> We are not used to open a CVE.
>
> [...]
> >   It's nearly impossible to solve issues without error reporting. Good
> >   error reporting saves admins time and money.
>
> Until now, the errors were reported on the list and most often fixed
> quickly.
> While I agree we need a more formal process (a bug tracker), I think we
> must
> be noticed of new bugs on the mailing list.
> Since nobody was against the bugzilla proposal, a deployment will be
> planned.
>         http://dpdk.org/ml/archives/dev/2015-August/023012.html
>

Re: [dpdk-dev] thoughts on DPDK after a few days of reading sources

[Dave Neary]

Hi,

On 02/11/2016 02:58 AM, Thomas Monjalon wrote:
> 2016-02-10 19:05, Seth Arnold:
> [...]
>>   It's nearly impossible to solve issues without error reporting. Good
>>   error reporting saves admins time and money.
> 
> Until now, the errors were reported on the list and most often fixed quickly.
> While I agree we need a more formal process (a bug tracker), I think we must
> be noticed of new bugs on the mailing list.
> Since nobody was against the bugzilla proposal, a deployment will be planned.
> 	http://dpdk.org/ml/archives/dev/2015-August/023012.html

I may have misunderstood Seth's comment, but it looked like he was
talking about checking errno after fopen and reporting the error with
perror or strerror in the event of a non-zero return.

Seth, did I understand correctly?

Thanks,
Dave.

-- 
Dave Neary - NFV/SDN Community Strategy
Open Source and Standards, Red Hat - http://community.redhat.com
Ph: +1-978-399-2182 / Cell: +1-978-799-3338

Re: [dpdk-dev] thoughts on DPDK after a few days of reading sources

[Thomas Monjalon]

2016-02-11 11:13, Dave Neary:
> Hi,
> 
> On 02/11/2016 02:58 AM, Thomas Monjalon wrote:
> > 2016-02-10 19:05, Seth Arnold:
> > [...]
> >>   It's nearly impossible to solve issues without error reporting. Good
> >>   error reporting saves admins time and money.
> > 
> > Until now, the errors were reported on the list and most often fixed quickly.
> > While I agree we need a more formal process (a bug tracker), I think we must
> > be noticed of new bugs on the mailing list.
> > Since nobody was against the bugzilla proposal, a deployment will be planned.
> > 	http://dpdk.org/ml/archives/dev/2015-August/023012.html
> 
> I may have misunderstood Seth's comment, but it looked like he was
> talking about checking errno after fopen and reporting the error with
> perror or strerror in the event of a non-zero return.

Maybe I misunderstood his comments.
Anyway, after looking at his list of bugs, it appears that a bug tracker
would be useful :)

Re: [dpdk-dev] thoughts on DPDK after a few days of reading sources

[Wiles, Keith]

>2016-02-11 11:13, Dave Neary:
>> Hi,
>> 
>> On 02/11/2016 02:58 AM, Thomas Monjalon wrote:
>> > 2016-02-10 19:05, Seth Arnold:
>> > [...]
>> >>   It's nearly impossible to solve issues without error reporting. Good
>> >>   error reporting saves admins time and money.
>> > 
>> > Until now, the errors were reported on the list and most often fixed quickly.
>> > While I agree we need a more formal process (a bug tracker), I think we must
>> > be noticed of new bugs on the mailing list.
>> > Since nobody was against the bugzilla proposal, a deployment will be planned.
>> > 	http://dpdk.org/ml/archives/dev/2015-August/023012.html
>> 
>> I may have misunderstood Seth's comment, but it looked like he was
>> talking about checking errno after fopen and reporting the error with
>> perror or strerror in the event of a non-zero return.
>
>Maybe I misunderstood his comments.
>Anyway, after looking at his list of bugs, it appears that a bug tracker
>would be useful :)

+1 for a bug tracker

I think some of these are reported by Coverity, correct?
>
>
>


Regards,
Keith

Re: [dpdk-dev] thoughts on DPDK after a few days of reading sources

[Stephen Hemminger]

On Wed, 10 Feb 2016 19:05:40 -0800
Seth Arnold <seth.arnold@canonical.com> wrote:

> - ./drivers/net/virtio/virtio_ethdev.c virtio_set_multiple_queues() calls
>   virtio_send_command(), which performs:
>   memcpy(vq->virtio_net_hdr_mz->addr, ctrl, sizeof(struct virtio_pmd_ctrl));
>   This copies a potentially huge amount of uninitialized data into ->addr
>   because the struct virtio_pmd_ctrl ctrl was not zeroed before being
>   passed. How much of this data leaves the system? Does this require a
>   CVE?

This is not really a security issue.
The guest (virtio) has to trust the host to follow the protocol.
If the host is malicious there are far worse things it can do.

In this case. The onstack variabl ctrl is only partially initialized
but only partially used. The hdr part (virtio_net_ctrl_hdr) is fully
initialized, and status is set to 0 in virtio_send_command.

Although partially unitialized data is copied into region shared with host,
only the first part is actually referenced by the ring element:

 	vq->vq_ring.desc[head].flags = VRING_DESC_F_NEXT;
	vq->vq_ring.desc[head].addr = vq->virtio_net_hdr_mz->phys_addr;
	vq->vq_ring.desc[head].len = sizeof(struct virtio_net_ctrl_hdr);

Therefore it is not a real problem.

Re: [dpdk-dev] thoughts on DPDK after a few days of reading sources

[Christian Ehrhardt]

On Thu, Feb 11, 2016 at 8:58 AM, Thomas Monjalon <thomas.monjalon@6wind.com>
wrote:

> Hi,
>
> 2016-02-10 19:05, Seth Arnold:
> > I've taken some notes while reading the sources; I'm sharing them in the
> > hopes that it's useful: on the one hand my fresh eyes may spot things
> that
> > you've overlooked, on the other hand your familiarity with the code means
> > that you're better suited to judge what I've found.
>
> Thanks for taking time and sharing, it's very valuable.
>
> > - shellcheck reports extensive cases of forgotten quotes to prevent word
> >   splitting or globbing, potentially unused variables, error-prone printf
> >   formatting. The scripts that are going to be used at runtime should be
> >   fixed:
> >   - ./debian/dpdk-init
> >   - ./debian/dpdk.init
>
> These files are not in the tree. Should they?
>
>
You are right, they are not in tree.
These few are parts of the ubuntu packaging and will be fixed by me this
week.
Sooner or later that might (partially?) flow back to the Debian packaging
that is in tree.


Christian Ehrhardt
Software Engineer, Ubuntu Server
Canonical Ltd

[...]

Re: [dpdk-dev] thoughts on DPDK after a few days of reading sources

[Christian Ehrhardt]

Hi Alejandro,
thanks for the heads up - to make sure I got your correctly I assume you
refer to:

[1]: dpdk:
http://dpdk.org/browse/dpdk/commit/?id=e61512e4066740847ced4a85ee9c3334b511468f
[2]: kernel:
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=033291eccbdb
[3]: kernel:
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=ae5515d66362b9d96cdcfce504567f0b8b7bd83e
[4]: kenrel:
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=03a76b60f8ba27974e2d252bc555d2c103420e15

It seems we will be shipping DPDK 2.2 and Kernel 4.4 with the next release
(no commitment being made).
[1] is not in DPDK 2.2, so it won't be supported right away. If it can be
used without [1] I wouldn't have realized that yet.
But also in the kernel it is not only disabled by default - in fact the
code itself got removed [3] and only later back into 4.5 [4].
I guess, to really activate, exploit, test and eventually support it - it
would be up to an explicit request to do so which can then be evaluated
against the risks.

IMHO I think we have to wait and see how it will be workin in DPDK >=16.x
and Kernel >=4.5 this kind of referring to [3] "... so rather than support
an unproven kernel interface revert it and revisit ...".

Thanks for the heads up Alejandro, I didn't really think much about it
before - I'll revisit it every now and then to see how development around
it goes on.


Christian Ehrhardt
Software Engineer, Ubuntu Server
Canonical Ltd

On Thu, Feb 11, 2016 at 12:58 PM, Alejandro Lucero <
alejandro.lucero@netronome.com> wrote:

> Hi Seth,
>
> I do not know if you and Ubuntu know about the kernel VFIO no-iommu mode
> which DPDK will use in the future (then getting rid of UIO drives).
>
> This implies distributions enabling that kernel VFIO mode which is not
> enable by default as it is a security issue.
>
> It would be good to know which is the Ubuntu position regarding this issue
> and if there are any date or plan for supporting this.
>
> Thanks
>
> On Thu, Feb 11, 2016 at 7:58 AM, Thomas Monjalon <
> thomas.monjalon@6wind.com>
> wrote:
>
> > Hi,
> >
> > 2016-02-10 19:05, Seth Arnold:
> > > I've taken some notes while reading the sources; I'm sharing them in
> the
> > > hopes that it's useful: on the one hand my fresh eyes may spot things
> > that
> > > you've overlooked, on the other hand your familiarity with the code
> means
> > > that you're better suited to judge what I've found.
> >
> > Thanks for taking time and sharing, it's very valuable.
> >
> > > - shellcheck reports extensive cases of forgotten quotes to prevent
> word
> > >   splitting or globbing, potentially unused variables, error-prone
> printf
> > >   formatting. The scripts that are going to be used at runtime should
> be
> > >   fixed:
> > >   - ./debian/dpdk-init
> > >   - ./debian/dpdk.init
> >
> > These files are not in the tree. Should they?
> >
> > > - ./drivers/net/cxgbe/cxgbe_ethdev.c eth_cxgbe_dev_init() memory leak
> in
> > >   out_free_adapter: that doesn't free adapter
> > > - ./drivers/net/virtio/virtio_ethdev.c virtio_set_multiple_queues()
> calls
> > >   virtio_send_command(), which performs:
> > >   memcpy(vq->virtio_net_hdr_mz->addr, ctrl, sizeof(struct
> > virtio_pmd_ctrl));
> > >   This copies a potentially huge amount of uninitialized data into
> ->addr
> > >   because the struct virtio_pmd_ctrl ctrl was not zeroed before being
> > >   passed. How much of this data leaves the system? Does this require a
> > >   CVE?
> >
> > We are not used to open a CVE.
> >
> > [...]
> > >   It's nearly impossible to solve issues without error reporting. Good
> > >   error reporting saves admins time and money.
> >
> > Until now, the errors were reported on the list and most often fixed
> > quickly.
> > While I agree we need a more formal process (a bug tracker), I think we
> > must
> > be noticed of new bugs on the mailing list.
> > Since nobody was against the bugzilla proposal, a deployment will be
> > planned.
> >         http://dpdk.org/ml/archives/dev/2015-August/023012.html
> >
>