On Tue, Jun 17, 2014 at 11:44:11AM +0200, Paolo Bonzini wrote:> Il 17/06/2014 11:03, David Marchand ha scritto: > >>Unless someone steps up and maintains ivshmem, I think it should be > >>deprecated and dropped from QEMU. > > > >Then I can maintain ivshmem for QEMU. > >If this is ok, I will send a patch for MAINTAINERS file. > > Typically, adding yourself to maintainers is done only after having proved > your ability to be a maintainer. :) > > So, let's stop talking and go back to code! You can start doing what was > suggested elsewhere in the thread: get the server and uio driver merged into > the QEMU tree, document the protocol in docs/specs/ivshmem_device_spec.txt, > and start fixing bugs such as the ones that Markus reported.One more thing to add to the list: static void ivshmem_read(void *opaque, const uint8_t * buf, int flags) The "flags" argument should be "size". Size should be checked before accessing buf. Please also see the bug fixes in the following unapplied patch: "[PATCH] ivshmem: fix potential OOB r/w access (#2)" by Sebastian Krahmer https://lists.gnu.org/archive/html/qemu-devel/2014-04/msg03538.html Stefan -------------- next part -------------- A non-text attachment was scrubbed... Name: not available Type: application/pgp-signature Size: 473 bytes Desc: not available URL: <http://lists.linuxfoundation.org/pipermail/virtualization/attachments/20140618/3cb9f30f/attachment.sig>
Hello Stefan, On 06/18/2014 12:48 PM, Stefan Hajnoczi wrote:> One more thing to add to the list: > > static void ivshmem_read(void *opaque, const uint8_t * buf, int flags) > > The "flags" argument should be "size". Size should be checked before > accessing buf.You are welcome to send a fix and I will review it.> > Please also see the bug fixes in the following unapplied patch: > "[PATCH] ivshmem: fix potential OOB r/w access (#2)" by Sebastian Krahmer > https://lists.gnu.org/archive/html/qemu-devel/2014-04/msg03538.htmlThanks for the pointer. I'll check it. -- David Marchand
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Am 18.06.2014 12:48, schrieb Stefan Hajnoczi:> On Tue, Jun 17, 2014 at 11:44:11AM +0200, Paolo Bonzini wrote: >> Il 17/06/2014 11:03, David Marchand ha scritto: >>>> Unless someone steps up and maintains ivshmem, I think it >>>> should be deprecated and dropped from QEMU. >>> >>> Then I can maintain ivshmem for QEMU. If this is ok, I will >>> send a patch for MAINTAINERS file. >> >> Typically, adding yourself to maintainers is done only after >> having proved your ability to be a maintainer. :) >> >> So, let's stop talking and go back to code! You can start doing >> what was suggested elsewhere in the thread: get the server and >> uio driver merged into the QEMU tree, document the protocol in >> docs/specs/ivshmem_device_spec.txt, and start fixing bugs such as >> the ones that Markus reported. > > One more thing to add to the list: > > static void ivshmem_read(void *opaque, const uint8_t * buf, int > flags) > > The "flags" argument should be "size". Size should be checked > before accessing buf. > > Please also see the bug fixes in the following unapplied patch: > "[PATCH] ivshmem: fix potential OOB r/w access (#2)" by Sebastian > Krahmer > https://lists.gnu.org/archive/html/qemu-devel/2014-04/msg03538.htmlJumping>late onto this thread: SUSE Security team has just recently done a thorough review of QEMU ivshmem code because a customer has requested this be supported in SLES12. Multiple security-related patches were submitted by Stefan Hajnoczi and Sebastian Krahmer, and I fear they are probably still not merged for lack of active maintainer... In such cases, after review, I expect them to be picked up by Peter as committer or via qemu-trivial. So -1, against dropping it. Vincent, you will find an RFC for an ivshmem-test in the qemu-devel list archives or possibly on my qtest branch. The blocking issue that I haven't worked on yet is that we can't unconditionally run the qtest because it depends on KVM enabled at configure time (as opposed to runtime) to have the device available. http://patchwork.ozlabs.org/patch/336367/ As others have stated before, the nahanni server seems unmaintained, thus not getting packaged by SUSE either and making testing the interrupt parts of ivshmem difficult - unless we sort out and fill with actual test code my proposed qtest. Regards, Andreas - -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N?rnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imend?rffer; HRB 16746 AG N?rnberg -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.22 (GNU/Linux) iQIcBAEBAgAGBQJToanZAAoJEPou0S0+fgE/L6YP/jtPiwvz3YoW3+H/h/YzrnE7 xVP92jj5orzmbG3HMmEnx0l7YrtzYkwymgUO56dy2SrLFe0xVMnxuzcHLzHLsnm3 bYvMVq3eAx8sdx9c/O2B/rQbNo2p8PF/luTNewN7A+w5TX0XgxdI3TpLT2pVxf0b kMaBnfivzUf2JY/zg6NaiGnwvVrA/0kXsCGKcTBiMQxOX2EdDgNak842SjlmS332 dPbqp5PIMdxwCxI/p+gpmu0cSy1bl2H6N2gkmKQZ63Z2tA7bWn/APdQeHyOcESZE xRAfDz2Cs3/6EL7FLirJWdwT9EMNaFcM+eRgIqDamFzviQPZVuLKdDUteO1k9x1s FlhL3ZRa3qHair9ByEJItqzneAeYmuwZ2DkKh4p/HQfbcxLzZlL8a1EEtYz5DTy0 8+Ax6IU5U5RZmwJ4/M/Ov5eT4t/fNe0MbG3mf5A8FJ6GWoF11ut/wyj70p/EmXua QjUblK/eFemN4YvIi0ovD4DR9ZH2+bXOb44wKL7yFahKLldaP4y9DhJTap2J0mT1 b62FfFZ6hVIGP5n30OHLlhe39QY6SyIPc4JNc9VZ3GcpXtfOHPUOAD/ykt/As1P3 cPfL+jM0QSb6VNJHNbvUsSlJ6xI26qEWzyJ5R7ww4fyEoq4XiE2RCDUWJ2t9/jQb +Bi/esBUDhAduc1Eh3FK =MtPH -----END PGP SIGNATURE-----
Il 18/06/2014 16:57, David Marchand ha scritto:> Hello Stefan, > > On 06/18/2014 12:48 PM, Stefan Hajnoczi wrote: >> One more thing to add to the list: >> >> static void ivshmem_read(void *opaque, const uint8_t * buf, int flags) >> >> The "flags" argument should be "size". Size should be checked before >> accessing buf. > > You are welcome to send a fix and I will review it.This is not what a maintainer should do. A maintainer should, if possible, contribute fixes to improve the code. I know this is very different from usual "company-style" development (even open source software can be developed on with methods more typical of proprietary software), but we're asking you to do it because you evidently understand ivshmem better than us. Claudio has more experience with free/open-source software. Since he's interested in ivshmem, he can help you too. Perhaps you could try sending out the patch, and Claudio can review it and send pull requests at least in the beginning? Paolo
On 06/18/2014 05:01 PM, Andreas F?rber wrote:> late onto this thread: SUSE Security team has just recently > done a thorough review of QEMU ivshmem code because a customer has > requested this be supported in SLES12. Multiple security-related > patches were submitted by Stefan Hajnoczi and Sebastian Krahmer, and I > fear they are probably still not merged for lack of active > maintainer... In such cases, after review, I expect them to be picked > up by Peter as committer or via qemu-trivial. > > So -1, against dropping it.Are these patches on patchwork ?> Vincent, you will find an RFC for an ivshmem-test in the qemu-devel > list archives or possibly on my qtest branch. The blocking issue that > I haven't worked on yet is that we can't unconditionally run the qtest > because it depends on KVM enabled at configure time (as opposed to > runtime) to have the device available. > http://patchwork.ozlabs.org/patch/336367/ > > As others have stated before, the nahanni server seems unmaintained, > thus not getting packaged by SUSE either and making testing the > interrupt parts of ivshmem difficult - unless we sort out and fill > with actual test code my proposed qtest.Thanks for the RFC patch. About ivshmem server, yes I will look at it. I will see what I can propose or if importing nahanni implementation as-is is the best solution. Anyway, first, documentation. -- David Marchand
On Wed, Jun 18, 2014 at 10:57 PM, David Marchand <david.marchand at 6wind.com> wrote:> On 06/18/2014 12:48 PM, Stefan Hajnoczi wrote: >> >> One more thing to add to the list: >> >> static void ivshmem_read(void *opaque, const uint8_t * buf, int flags) >> >> The "flags" argument should be "size". Size should be checked before >> accessing buf. > > > You are welcome to send a fix and I will review it.I don't plan to send ivshmem patches in the near future because I don't use or support it. I thought you were interested in bringing ivshmem up to a level where distros feel comfortable enabling and supporting it. Getting there will require effort from you to audit, clean up, and achieve test coverage. That's what a maintainer needs to do in a case like this. Stefan
Stefan Hajnoczi <stefanha at gmail.com> writes:> On Tue, Jun 17, 2014 at 11:44:11AM +0200, Paolo Bonzini wrote: >> Il 17/06/2014 11:03, David Marchand ha scritto: >> >>Unless someone steps up and maintains ivshmem, I think it should be >> >>deprecated and dropped from QEMU. >> > >> >Then I can maintain ivshmem for QEMU. >> >If this is ok, I will send a patch for MAINTAINERS file. >> >> Typically, adding yourself to maintainers is done only after having proved >> your ability to be a maintainer. :) >> >> So, let's stop talking and go back to code! You can start doing what was >> suggested elsewhere in the thread: get the server and uio driver merged into >> the QEMU tree, document the protocol in docs/specs/ivshmem_device_spec.txt, >> and start fixing bugs such as the ones that Markus reported. > > One more thing to add to the list: > > static void ivshmem_read(void *opaque, const uint8_t * buf, int flags) > > The "flags" argument should be "size". Size should be checked before > accessing buf. > > Please also see the bug fixes in the following unapplied patch: > "[PATCH] ivshmem: fix potential OOB r/w access (#2)" by Sebastian Krahmer > https://lists.gnu.org/archive/html/qemu-devel/2014-04/msg03538.htmlAnother one: most devices can be controlled via a dedicated CONFIG_<DEVNAME>, but not ivshmem: it uses CONFIG_KVM and CONFIG_PCI. Giving it its own CONFIG_IVSHMEM would be nice.