On Fri, Oct 18, 2019 at 11:54:58AM -0700, Greg Kroah-Hartman wrote:
On Fri, Oct 18, 2019 at 05:19:55PM +0200, Johan Hovold wrote:
The custom ring-buffer implementation was merged without any locking whatsoever, but a spinlock was later added by commit 9d33efd9a791 ("USB: ldusb bugfix").
The lock did not cover the loads from the ring-buffer entry after determining the buffer was non-empty, nor the update of the tail index once the entry had been processed. The former could lead to stale data being returned, while the latter could lead to memory corruption on sufficiently weakly ordered architectures.
Ugh.
This almost looks sane, but what's the odds there is some other issue in here as well? Would it make sense to just convert the code to use the "standard" ring buffer code instead?
Yeah, long term that may be the right thing to do, but I wanted a minimal fix addressing the issue at hand without having to reimplement the driver and fix all other (less-critical) issues in there...
For the ring-buffer corruption / info-leak issue, these two patches should be sufficient though.
Copying the ring-buffer entry to a temporary buffer while holding the lock might still be preferred to avoid having to deal with barrier subtleties. But unless someone speaks out against 2/2, I'd just go ahead and apply it.
Johan