New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

staging: vchiq_arm: fix msgbufcount in VCHIQ_IOC_AWAIT_COMPLETION compat ioctl #2703

Merged
merged 1 commit into from Nov 13, 2018

Conversation

Projects
None yet
4 participants
@lopsided98
Contributor

lopsided98 commented Oct 5, 2018

The compatibility ioctl wrapper for VCHIQ_IOC_AWAIT_COMPLETION assumes that
the native ioctl always uses a message buffer and decrements msgbufcount.
Certain message types do not use a message buffer and in this case
msgbufcount is not decremented, and completion->header for the message is
NULL. Because the wrapper unconditionally decrements msgbufcount, the
calling process may assume that a message buffer has been used even when
it has not.

This bug results in a memory leak in vchiq_lib. When msgbufcount is decremented, the code assumes
that the buffer can be freed though the reference in completion->header,
which cannot happen when the reference is NULL. In my case, I ran into this bug when attempting to use gst-omx, as described here: https://bugzilla.gnome.org/show_bug.cgi?id=797159

This patch causes the wrapper to only decrement msgbufcount when the
native ioctl decrements it. Note that we cannot simply copy the native
ioctl's value of msgbufcount, because the wrapper only retrieves messages
from the native ioctl one at a time, while userspace may request multiple
messages.

@lategoodbye

This comment has been minimized.

Contributor

lategoodbye commented Oct 6, 2018

It came to my thoughts, that it looks a bit strange that we need to fix the kernel in order to avoid memory leaks in userspace. But i don't have a deep look into vchiq.

In case this is the proper fix, here are my suggestions (upstreaming):

  • msgbufcounttemp isn't the best variable name
  • add Fixes: 5569a12 ("staging: vchiq_arm: Add compatibility wrappers for ioctls") to commit log
@lopsided98

This comment has been minimized.

Contributor

lopsided98 commented Oct 6, 2018

This is what happens to cause the memory leak:

  1. The ioctl is called with args.msgbufcount == 8 from completion_thread
  2. Compat ioctl calls native ioctl with native_args.msgbufcount == 1 (native_args is not called that in the code, I'm just using the name to distinguish from the args passed from userspace)
  3. Native ioctl returns a completion for a message which does not use a buffer
  4. completion->header == NULL. If the message had used a buffer, completion->header would point at one of the message buffers
  5. native_args.msgbufcount == 1 (no buffer was used, so the count does not change)
  6. args.msgbufcount is unconditionally decremented, so now args.msgbufcount == 7 (this is what my patch changes)
  7. Return to userspace, call a callback for each received completion
  8. Callback puts message in queue which gets processed here
  9. Attempt to "free" (actually return to a pool) the buffer. The buffer is NULL so nothing happens. If the buffer had not been NULL, it would have been returned to the pool to be reused later.
  10. Return to beginning of loop in completion_thread.
  11. Because args.msgbufcount == 7 < 8, that indicates that a buffer was passed to the callback and either a buffer needs to be taken from the pool or a new one needs to be allocated to replace the buffer that may be still in use by the callback. Because no buffer was passed to the callback, the replaced buffer will never be returned to the pool, and this will always allocate a new buffer, resulting in a memory leak.

@lopsided98 lopsided98 force-pushed the lopsided98:vchiq-ioctl-fix branch from c19f818 to b9807c4 Oct 6, 2018

@lopsided98

This comment has been minimized.

Contributor

lopsided98 commented Oct 6, 2018

I'm not sure what else to call msgbufcounttemp. It is just a temporary variable used to read a value in userspace. The same naming convention is used for completiontemp.

@lategoodbye

This comment has been minimized.

Contributor

lategoodbye commented Oct 7, 2018

Thank you very much for your explanations.

Good naming of variables could be difficult. One of the reasons why the vchiq driver is still in staging is because of the bad coding style. I would suggest to use msgbufcount_user or count_user. I know it's just nitpicking, but it's important that this code is easy to review.

Please understand this only as an invitation to upstream your bugfix afterwards.

@lopsided98 lopsided98 force-pushed the lopsided98:vchiq-ioctl-fix branch from b9807c4 to b1bdf77 Oct 7, 2018

@lopsided98

This comment has been minimized.

Contributor

lopsided98 commented Oct 7, 2018

Yes, I agree that the variable names could be better. It actually made it somewhat difficult to debug this problem.

I changed the name to msgbufcount_native to reflect the fact that it is returned from the native ioctl.

@lategoodbye

This comment has been minimized.

Contributor

lategoodbye commented Oct 8, 2018

Thanks

@lategoodbye

This comment has been minimized.

Contributor

lategoodbye commented Oct 16, 2018

@pelwell Any objections?

@pelwell

This comment has been minimized.

Contributor

pelwell commented Oct 17, 2018

Looks good to me.

@pelwell

This comment has been minimized.

Contributor

pelwell commented Oct 17, 2018

The patch modifies an upstream file. Report back when your patch is accepted by the kernel maintainers.

@lategoodbye

This comment has been minimized.

Contributor

lategoodbye commented Oct 17, 2018

@lopsided98 Are you familiar with sending a patch upstream? In case you have any questions, feel free to contact me: stefan.wahren@i2se.com

@lopsided98 lopsided98 force-pushed the lopsided98:vchiq-ioctl-fix branch 4 times, most recently from 98795ec to 62eae7f Nov 2, 2018

lopsided98 added a commit to lopsided98/linux-raspberrypi that referenced this pull request Nov 3, 2018

staging: vchiq_arm: fix compat VCHIQ_IOC_AWAIT_COMPLETION
The compatibility ioctl wrapper for VCHIQ_IOC_AWAIT_COMPLETION assumes that
the native ioctl always uses a message buffer and decrements msgbufcount.
Certain message types do not use a message buffer and in this case
msgbufcount is not decremented, and completion->header for the message is
NULL. Because the wrapper unconditionally decrements msgbufcount, the
calling process may assume that a message buffer has been used even when
it has not.

This results in a memory leak in the userspace code that interfaces with
this driver. When msgbufcount is decremented, the userspace code assumes
that the buffer can be freed though the reference in completion->header,
which cannot happen when the reference is NULL.

This patch causes the wrapper to only decrement msgbufcount when the
native ioctl decrements it. Note that we cannot simply copy the native
ioctl's value of msgbufcount, because the wrapper only retrieves messages
from the native ioctl one at a time, while userspace may request multiple
messages.

See raspberrypi#2703 for more discussion of
patch.

Fixes: 5569a12 ("staging: vchiq_arm: Add compatibility wrappers for ioctls")

Signed-off-by: Ben Wolsieffer <benwolsieffer@gmail.com>
staging: vchiq_arm: fix compat VCHIQ_IOC_AWAIT_COMPLETION
The compatibility ioctl wrapper for VCHIQ_IOC_AWAIT_COMPLETION assumes that
the native ioctl always uses a message buffer and decrements msgbufcount.
Certain message types do not use a message buffer and in this case
msgbufcount is not decremented, and completion->header for the message is
NULL. Because the wrapper unconditionally decrements msgbufcount, the
calling process may assume that a message buffer has been used even when
it has not.

This results in a memory leak in the userspace code that interfaces with
this driver. When msgbufcount is decremented, the userspace code assumes
that the buffer can be freed though the reference in completion->header,
which cannot happen when the reference is NULL.

This patch causes the wrapper to only decrement msgbufcount when the
native ioctl decrements it. Note that we cannot simply copy the native
ioctl's value of msgbufcount, because the wrapper only retrieves messages
from the native ioctl one at a time, while userspace may request multiple
messages.

See #2703 for more discussion of
this patch.

Fixes: 5569a12 ("staging: vchiq_arm: Add compatibility wrappers for ioctls")

Signed-off-by: Ben Wolsieffer <benwolsieffer@gmail.com>

@lopsided98 lopsided98 force-pushed the lopsided98:vchiq-ioctl-fix branch from 62eae7f to d44231d Nov 3, 2018

fengguang pushed a commit to 0day-ci/linux that referenced this pull request Nov 4, 2018

staging: vchiq_arm: fix compat VCHIQ_IOC_AWAIT_COMPLETION
The compatibility ioctl wrapper for VCHIQ_IOC_AWAIT_COMPLETION assumes that
the native ioctl always uses a message buffer and decrements msgbufcount.
Certain message types do not use a message buffer and in this case
msgbufcount is not decremented, and completion->header for the message is
NULL. Because the wrapper unconditionally decrements msgbufcount, the
calling process may assume that a message buffer has been used even when
it has not.

This results in a memory leak in the userspace code that interfaces with
this driver. When msgbufcount is decremented, the userspace code assumes
that the buffer can be freed though the reference in completion->header,
which cannot happen when the reference is NULL.

This patch causes the wrapper to only decrement msgbufcount when the
native ioctl decrements it. Note that we cannot simply copy the native
ioctl's value of msgbufcount, because the wrapper only retrieves messages
from the native ioctl one at a time, while userspace may request multiple
messages.

See raspberrypi/linux#2703 for more discussion of
this patch.

Fixes: 5569a12 ("staging: vchiq_arm: Add compatibility wrappers for ioctls")

Signed-off-by: Ben Wolsieffer <benwolsieffer@gmail.com>
@lategoodbye

This comment has been minimized.

Contributor

lategoodbye commented Nov 13, 2018

The patch has been applied to staging-linus and will be included in 4.20 + stable.

@popcornmix popcornmix merged commit d1394b5 into raspberrypi:rpi-4.14.y Nov 13, 2018

popcornmix added a commit that referenced this pull request Nov 13, 2018

staging: vchiq_arm: fix compat VCHIQ_IOC_AWAIT_COMPLETION (#2703)
The compatibility ioctl wrapper for VCHIQ_IOC_AWAIT_COMPLETION assumes that
the native ioctl always uses a message buffer and decrements msgbufcount.
Certain message types do not use a message buffer and in this case
msgbufcount is not decremented, and completion->header for the message is
NULL. Because the wrapper unconditionally decrements msgbufcount, the
calling process may assume that a message buffer has been used even when
it has not.

This results in a memory leak in the userspace code that interfaces with
this driver. When msgbufcount is decremented, the userspace code assumes
that the buffer can be freed though the reference in completion->header,
which cannot happen when the reference is NULL.

This patch causes the wrapper to only decrement msgbufcount when the
native ioctl decrements it. Note that we cannot simply copy the native
ioctl's value of msgbufcount, because the wrapper only retrieves messages
from the native ioctl one at a time, while userspace may request multiple
messages.

See #2703 for more discussion of
this patch.

Fixes: 5569a12 ("staging: vchiq_arm: Add compatibility wrappers for ioctls")

Signed-off-by: Ben Wolsieffer <benwolsieffer@gmail.com>

ahmedradaideh added a commit to ahmedradaideh/Pi-Kernel that referenced this pull request Nov 13, 2018

staging: vchiq_arm: fix compat VCHIQ_IOC_AWAIT_COMPLETION (#2703)
The compatibility ioctl wrapper for VCHIQ_IOC_AWAIT_COMPLETION assumes that
the native ioctl always uses a message buffer and decrements msgbufcount.
Certain message types do not use a message buffer and in this case
msgbufcount is not decremented, and completion->header for the message is
NULL. Because the wrapper unconditionally decrements msgbufcount, the
calling process may assume that a message buffer has been used even when
it has not.

This results in a memory leak in the userspace code that interfaces with
this driver. When msgbufcount is decremented, the userspace code assumes
that the buffer can be freed though the reference in completion->header,
which cannot happen when the reference is NULL.

This patch causes the wrapper to only decrement msgbufcount when the
native ioctl decrements it. Note that we cannot simply copy the native
ioctl's value of msgbufcount, because the wrapper only retrieves messages
from the native ioctl one at a time, while userspace may request multiple
messages.

See raspberrypi/linux#2703 for more discussion of
this patch.

Fixes: 5569a12 ("staging: vchiq_arm: Add compatibility wrappers for ioctls")

Signed-off-by: Ben Wolsieffer <benwolsieffer@gmail.com>
Signed-off-by: ahmedradaideh <ahmed.radaideh@gmail.com>

sergey-senozhatsky pushed a commit to sergey-senozhatsky/linux-next-ss that referenced this pull request Nov 14, 2018

staging: vchiq_arm: fix compat VCHIQ_IOC_AWAIT_COMPLETION
The compatibility ioctl wrapper for VCHIQ_IOC_AWAIT_COMPLETION assumes that
the native ioctl always uses a message buffer and decrements msgbufcount.
Certain message types do not use a message buffer and in this case
msgbufcount is not decremented, and completion->header for the message is
NULL. Because the wrapper unconditionally decrements msgbufcount, the
calling process may assume that a message buffer has been used even when
it has not.

This results in a memory leak in the userspace code that interfaces with
this driver. When msgbufcount is decremented, the userspace code assumes
that the buffer can be freed though the reference in completion->header,
which cannot happen when the reference is NULL.

This patch causes the wrapper to only decrement msgbufcount when the
native ioctl decrements it. Note that we cannot simply copy the native
ioctl's value of msgbufcount, because the wrapper only retrieves messages
from the native ioctl one at a time, while userspace may request multiple
messages.

See raspberrypi/linux#2703 for more discussion of
this patch.

Fixes: 5569a12 ("staging: vchiq_arm: Add compatibility wrappers for ioctls")
Signed-off-by: Ben Wolsieffer <benwolsieffer@gmail.com>
Acked-by: Stefan Wahren <stefan.wahren@i2se.com>
Cc: stable <stable@vger.kernel.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

pelwell added a commit that referenced this pull request Nov 14, 2018

staging: vchiq_arm: fix compat VCHIQ_IOC_AWAIT_COMPLETION (#2703)
The compatibility ioctl wrapper for VCHIQ_IOC_AWAIT_COMPLETION assumes that
the native ioctl always uses a message buffer and decrements msgbufcount.
Certain message types do not use a message buffer and in this case
msgbufcount is not decremented, and completion->header for the message is
NULL. Because the wrapper unconditionally decrements msgbufcount, the
calling process may assume that a message buffer has been used even when
it has not.

This results in a memory leak in the userspace code that interfaces with
this driver. When msgbufcount is decremented, the userspace code assumes
that the buffer can be freed though the reference in completion->header,
which cannot happen when the reference is NULL.

This patch causes the wrapper to only decrement msgbufcount when the
native ioctl decrements it. Note that we cannot simply copy the native
ioctl's value of msgbufcount, because the wrapper only retrieves messages
from the native ioctl one at a time, while userspace may request multiple
messages.

See #2703 for more discussion of
this patch.

Fixes: 5569a12 ("staging: vchiq_arm: Add compatibility wrappers for ioctls")

Signed-off-by: Ben Wolsieffer <benwolsieffer@gmail.com>

popcornmix added a commit that referenced this pull request Nov 15, 2018

staging: vchiq_arm: fix compat VCHIQ_IOC_AWAIT_COMPLETION (#2703)
The compatibility ioctl wrapper for VCHIQ_IOC_AWAIT_COMPLETION assumes that
the native ioctl always uses a message buffer and decrements msgbufcount.
Certain message types do not use a message buffer and in this case
msgbufcount is not decremented, and completion->header for the message is
NULL. Because the wrapper unconditionally decrements msgbufcount, the
calling process may assume that a message buffer has been used even when
it has not.

This results in a memory leak in the userspace code that interfaces with
this driver. When msgbufcount is decremented, the userspace code assumes
that the buffer can be freed though the reference in completion->header,
which cannot happen when the reference is NULL.

This patch causes the wrapper to only decrement msgbufcount when the
native ioctl decrements it. Note that we cannot simply copy the native
ioctl's value of msgbufcount, because the wrapper only retrieves messages
from the native ioctl one at a time, while userspace may request multiple
messages.

See #2703 for more discussion of
this patch.

Fixes: 5569a12 ("staging: vchiq_arm: Add compatibility wrappers for ioctls")

Signed-off-by: Ben Wolsieffer <benwolsieffer@gmail.com>

@lopsided98 lopsided98 deleted the lopsided98:vchiq-ioctl-fix branch Nov 18, 2018

popcornmix added a commit that referenced this pull request Nov 19, 2018

staging: vchiq_arm: fix compat VCHIQ_IOC_AWAIT_COMPLETION (#2703)
The compatibility ioctl wrapper for VCHIQ_IOC_AWAIT_COMPLETION assumes that
the native ioctl always uses a message buffer and decrements msgbufcount.
Certain message types do not use a message buffer and in this case
msgbufcount is not decremented, and completion->header for the message is
NULL. Because the wrapper unconditionally decrements msgbufcount, the
calling process may assume that a message buffer has been used even when
it has not.

This results in a memory leak in the userspace code that interfaces with
this driver. When msgbufcount is decremented, the userspace code assumes
that the buffer can be freed though the reference in completion->header,
which cannot happen when the reference is NULL.

This patch causes the wrapper to only decrement msgbufcount when the
native ioctl decrements it. Note that we cannot simply copy the native
ioctl's value of msgbufcount, because the wrapper only retrieves messages
from the native ioctl one at a time, while userspace may request multiple
messages.

See #2703 for more discussion of
this patch.

Fixes: 5569a12 ("staging: vchiq_arm: Add compatibility wrappers for ioctls")

Signed-off-by: Ben Wolsieffer <benwolsieffer@gmail.com>

popcornmix added a commit to raspberrypi/firmware that referenced this pull request Nov 19, 2018

kernel: Bump to 4.14.81
kernel: vcsm: Fix an NULL dereference in the import_dmabuf error path
See: raspberrypi/linux#2753

overlays: Remove superfluous #address/size-cells

kernel: staging: vchiq_arm: fix msgbufcount in VCHIQ_IOC_AWAIT_COMPLETION compat ioctl
See: raspberrypi/linux#2703

popcornmix added a commit to Hexxeh/rpi-firmware that referenced this pull request Nov 19, 2018

kernel: Bump to 4.14.81
kernel: vcsm: Fix an NULL dereference in the import_dmabuf error path
See: raspberrypi/linux#2753

overlays: Remove superfluous #address/size-cells

kernel: staging: vchiq_arm: fix msgbufcount in VCHIQ_IOC_AWAIT_COMPLETION compat ioctl
See: raspberrypi/linux#2703

popcornmix added a commit that referenced this pull request Nov 21, 2018

staging: vchiq_arm: fix compat VCHIQ_IOC_AWAIT_COMPLETION (#2703)
The compatibility ioctl wrapper for VCHIQ_IOC_AWAIT_COMPLETION assumes that
the native ioctl always uses a message buffer and decrements msgbufcount.
Certain message types do not use a message buffer and in this case
msgbufcount is not decremented, and completion->header for the message is
NULL. Because the wrapper unconditionally decrements msgbufcount, the
calling process may assume that a message buffer has been used even when
it has not.

This results in a memory leak in the userspace code that interfaces with
this driver. When msgbufcount is decremented, the userspace code assumes
that the buffer can be freed though the reference in completion->header,
which cannot happen when the reference is NULL.

This patch causes the wrapper to only decrement msgbufcount when the
native ioctl decrements it. Note that we cannot simply copy the native
ioctl's value of msgbufcount, because the wrapper only retrieves messages
from the native ioctl one at a time, while userspace may request multiple
messages.

See #2703 for more discussion of
this patch.

Fixes: 5569a12 ("staging: vchiq_arm: Add compatibility wrappers for ioctls")

Signed-off-by: Ben Wolsieffer <benwolsieffer@gmail.com>

popcornmix added a commit that referenced this pull request Nov 23, 2018

staging: vchiq_arm: fix compat VCHIQ_IOC_AWAIT_COMPLETION (#2703)
The compatibility ioctl wrapper for VCHIQ_IOC_AWAIT_COMPLETION assumes that
the native ioctl always uses a message buffer and decrements msgbufcount.
Certain message types do not use a message buffer and in this case
msgbufcount is not decremented, and completion->header for the message is
NULL. Because the wrapper unconditionally decrements msgbufcount, the
calling process may assume that a message buffer has been used even when
it has not.

This results in a memory leak in the userspace code that interfaces with
this driver. When msgbufcount is decremented, the userspace code assumes
that the buffer can be freed though the reference in completion->header,
which cannot happen when the reference is NULL.

This patch causes the wrapper to only decrement msgbufcount when the
native ioctl decrements it. Note that we cannot simply copy the native
ioctl's value of msgbufcount, because the wrapper only retrieves messages
from the native ioctl one at a time, while userspace may request multiple
messages.

See #2703 for more discussion of
this patch.

Fixes: 5569a12 ("staging: vchiq_arm: Add compatibility wrappers for ioctls")

Signed-off-by: Ben Wolsieffer <benwolsieffer@gmail.com>

popcornmix added a commit that referenced this pull request Nov 28, 2018

staging: vchiq_arm: fix compat VCHIQ_IOC_AWAIT_COMPLETION (#2703)
The compatibility ioctl wrapper for VCHIQ_IOC_AWAIT_COMPLETION assumes that
the native ioctl always uses a message buffer and decrements msgbufcount.
Certain message types do not use a message buffer and in this case
msgbufcount is not decremented, and completion->header for the message is
NULL. Because the wrapper unconditionally decrements msgbufcount, the
calling process may assume that a message buffer has been used even when
it has not.

This results in a memory leak in the userspace code that interfaces with
this driver. When msgbufcount is decremented, the userspace code assumes
that the buffer can be freed though the reference in completion->header,
which cannot happen when the reference is NULL.

This patch causes the wrapper to only decrement msgbufcount when the
native ioctl decrements it. Note that we cannot simply copy the native
ioctl's value of msgbufcount, because the wrapper only retrieves messages
from the native ioctl one at a time, while userspace may request multiple
messages.

See #2703 for more discussion of
this patch.

Fixes: 5569a12 ("staging: vchiq_arm: Add compatibility wrappers for ioctls")

Signed-off-by: Ben Wolsieffer <benwolsieffer@gmail.com>

popcornmix added a commit that referenced this pull request Nov 28, 2018

staging: vchiq_arm: fix compat VCHIQ_IOC_AWAIT_COMPLETION (#2703)
The compatibility ioctl wrapper for VCHIQ_IOC_AWAIT_COMPLETION assumes that
the native ioctl always uses a message buffer and decrements msgbufcount.
Certain message types do not use a message buffer and in this case
msgbufcount is not decremented, and completion->header for the message is
NULL. Because the wrapper unconditionally decrements msgbufcount, the
calling process may assume that a message buffer has been used even when
it has not.

This results in a memory leak in the userspace code that interfaces with
this driver. When msgbufcount is decremented, the userspace code assumes
that the buffer can be freed though the reference in completion->header,
which cannot happen when the reference is NULL.

This patch causes the wrapper to only decrement msgbufcount when the
native ioctl decrements it. Note that we cannot simply copy the native
ioctl's value of msgbufcount, because the wrapper only retrieves messages
from the native ioctl one at a time, while userspace may request multiple
messages.

See #2703 for more discussion of
this patch.

Fixes: 5569a12 ("staging: vchiq_arm: Add compatibility wrappers for ioctls")

Signed-off-by: Ben Wolsieffer <benwolsieffer@gmail.com>

lyakh added a commit to lyakh/linux that referenced this pull request Nov 30, 2018

staging: vchiq_arm: fix compat VCHIQ_IOC_AWAIT_COMPLETION (#2703)
The compatibility ioctl wrapper for VCHIQ_IOC_AWAIT_COMPLETION assumes that
the native ioctl always uses a message buffer and decrements msgbufcount.
Certain message types do not use a message buffer and in this case
msgbufcount is not decremented, and completion->header for the message is
NULL. Because the wrapper unconditionally decrements msgbufcount, the
calling process may assume that a message buffer has been used even when
it has not.

This results in a memory leak in the userspace code that interfaces with
this driver. When msgbufcount is decremented, the userspace code assumes
that the buffer can be freed though the reference in completion->header,
which cannot happen when the reference is NULL.

This patch causes the wrapper to only decrement msgbufcount when the
native ioctl decrements it. Note that we cannot simply copy the native
ioctl's value of msgbufcount, because the wrapper only retrieves messages
from the native ioctl one at a time, while userspace may request multiple
messages.

See raspberrypi/linux#2703 for more discussion of
this patch.

Fixes: 5569a12 ("staging: vchiq_arm: Add compatibility wrappers for ioctls")

Signed-off-by: Ben Wolsieffer <benwolsieffer@gmail.com>

popcornmix added a commit that referenced this pull request Dec 4, 2018

staging: vchiq_arm: fix compat VCHIQ_IOC_AWAIT_COMPLETION (#2703)
The compatibility ioctl wrapper for VCHIQ_IOC_AWAIT_COMPLETION assumes that
the native ioctl always uses a message buffer and decrements msgbufcount.
Certain message types do not use a message buffer and in this case
msgbufcount is not decremented, and completion->header for the message is
NULL. Because the wrapper unconditionally decrements msgbufcount, the
calling process may assume that a message buffer has been used even when
it has not.

This results in a memory leak in the userspace code that interfaces with
this driver. When msgbufcount is decremented, the userspace code assumes
that the buffer can be freed though the reference in completion->header,
which cannot happen when the reference is NULL.

This patch causes the wrapper to only decrement msgbufcount when the
native ioctl decrements it. Note that we cannot simply copy the native
ioctl's value of msgbufcount, because the wrapper only retrieves messages
from the native ioctl one at a time, while userspace may request multiple
messages.

See #2703 for more discussion of
this patch.

Fixes: 5569a12 ("staging: vchiq_arm: Add compatibility wrappers for ioctls")

Signed-off-by: Ben Wolsieffer <benwolsieffer@gmail.com>

woodsts pushed a commit to woodsts/linux-stable that referenced this pull request Dec 5, 2018

staging: vchiq_arm: fix compat VCHIQ_IOC_AWAIT_COMPLETION
commit 5a96b2d upstream.

The compatibility ioctl wrapper for VCHIQ_IOC_AWAIT_COMPLETION assumes that
the native ioctl always uses a message buffer and decrements msgbufcount.
Certain message types do not use a message buffer and in this case
msgbufcount is not decremented, and completion->header for the message is
NULL. Because the wrapper unconditionally decrements msgbufcount, the
calling process may assume that a message buffer has been used even when
it has not.

This results in a memory leak in the userspace code that interfaces with
this driver. When msgbufcount is decremented, the userspace code assumes
that the buffer can be freed though the reference in completion->header,
which cannot happen when the reference is NULL.

This patch causes the wrapper to only decrement msgbufcount when the
native ioctl decrements it. Note that we cannot simply copy the native
ioctl's value of msgbufcount, because the wrapper only retrieves messages
from the native ioctl one at a time, while userspace may request multiple
messages.

See raspberrypi/linux#2703 for more discussion of
this patch.

Fixes: 5569a12 ("staging: vchiq_arm: Add compatibility wrappers for ioctls")
Signed-off-by: Ben Wolsieffer <benwolsieffer@gmail.com>
Acked-by: Stefan Wahren <stefan.wahren@i2se.com>
Cc: stable <stable@vger.kernel.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

woodsts pushed a commit to woodsts/linux-stable that referenced this pull request Dec 5, 2018

staging: vchiq_arm: fix compat VCHIQ_IOC_AWAIT_COMPLETION
commit 5a96b2d upstream.

The compatibility ioctl wrapper for VCHIQ_IOC_AWAIT_COMPLETION assumes that
the native ioctl always uses a message buffer and decrements msgbufcount.
Certain message types do not use a message buffer and in this case
msgbufcount is not decremented, and completion->header for the message is
NULL. Because the wrapper unconditionally decrements msgbufcount, the
calling process may assume that a message buffer has been used even when
it has not.

This results in a memory leak in the userspace code that interfaces with
this driver. When msgbufcount is decremented, the userspace code assumes
that the buffer can be freed though the reference in completion->header,
which cannot happen when the reference is NULL.

This patch causes the wrapper to only decrement msgbufcount when the
native ioctl decrements it. Note that we cannot simply copy the native
ioctl's value of msgbufcount, because the wrapper only retrieves messages
from the native ioctl one at a time, while userspace may request multiple
messages.

See raspberrypi/linux#2703 for more discussion of
this patch.

Fixes: 5569a12 ("staging: vchiq_arm: Add compatibility wrappers for ioctls")
Signed-off-by: Ben Wolsieffer <benwolsieffer@gmail.com>
Acked-by: Stefan Wahren <stefan.wahren@i2se.com>
Cc: stable <stable@vger.kernel.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment