staging: vchiq_arm: fix msgbufcount in VCHIQ_IOC_AWAIT_COMPLETION compat ioctl

[lopsided98]

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]

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 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]

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]

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]

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]

Thanks

[lategoodbye]

@pelwell Any objections?

[pelwell]

Looks good to me.

[pelwell]

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

[lategoodbye]

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

[lategoodbye]

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