In case of a disconnect an ongoing flush() has to be made fail.
Nevertheless we cannot be sure that any pending URB has already
finished, so although they will never succeed, they still must
not be touched.
The clean solution for this is to check for WDM_IN_USE
and WDM_DISCONNECTED in flush(). There is no point in ever
clearing WDM_IN_USE, as no further writes make sense.
The issue is as old as the driver.
Fixes: afba937e54 ("USB: CDC WDM driver")
Reported-by: [email protected]
Signed-off-by: Oliver Neukum <[email protected]>
Cc: stable <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
Signed-off-by: Greg Kroah-Hartman <[email protected]>
The ioctl handler uses the intfdata of a second interface,
which may not be present in a broken or malicious device, hence
the intfdata needs to be checked for NULL.
[[email protected]: fix newly added spurious space]
Reported-by: [email protected]
Signed-off-by: Oliver Neukum <[email protected]>
Signed-off-by: Jiri Kosina <[email protected]>
The driver should check whether the endpoint it uses has the correct
type.
Reported-by: [email protected]
Signed-off-by: Oliver Neukum <[email protected]>
Signed-off-by: Dmitry Torokhov <[email protected]>
This converts the driver to using the BIT macro to increase readability
Signed-off-by: Oliver Neukum <[email protected]>
Signed-off-by: Hans Verkuil <[email protected]>
Signed-off-by: Mauro Carvalho Chehab <[email protected]>
Kasan reported a use after free in cpia2_usb_disconnect()
It first freed everything and then woke up those waiting.
The reverse order is correct.
Fixes: 6c493f8b28 ("[media] cpia2: major overhaul to get it in a working state again")
Signed-off-by: Oliver Neukum <[email protected]>
Reported-by: [email protected]
Signed-off-by: Hans Verkuil <[email protected]>
Signed-off-by: Mauro Carvalho Chehab <[email protected]>
dvb_usb_device_exit() frees and uses the device name in that order.
Fix by storing the name in a buffer before freeing it.
Signed-off-by: Oliver Neukum <[email protected]>
Reported-by: [email protected]
Signed-off-by: Sean Young <[email protected]>
Signed-off-by: Mauro Carvalho Chehab <[email protected]>
Added the newly added limit and updated the text a bit
Signed-off-by: Oliver Neukum <[email protected]>
Signed-off-by: Greg Kroah-Hartman <[email protected]>
Admitting that there can be only one device allows us to drop any
pretense about locking one device or a table of devices.
Signed-off-by: Oliver Neukum <[email protected]>
Signed-off-by: Greg Kroah-Hartman <[email protected]>
If a disconnected device is closed, rio_close() must free
the buffers.
Signed-off-by: Oliver Neukum <[email protected]>
Cc: stable <[email protected]>
Signed-off-by: Greg Kroah-Hartman <[email protected]>
This driver is using a global variable. It cannot handle more than
one device at a time. The issue has been existing since the dawn
of the driver.
Signed-off-by: Oliver Neukum <[email protected]>
Reported-by: [email protected]
Cc: stable <[email protected]>
Signed-off-by: Greg Kroah-Hartman <[email protected]>
The pointer used to log a failure of usb_register_dev() must
be set before the error is logged.
v2: fix that minor is not available before registration
Signed-off-by: oliver Neukum <[email protected]>
Reported-by: [email protected]
Fixes: 7b5cd5fefb ("USB: SisUSB2VGA: Convert printk to dev_* macros")
Cc: stable <[email protected]>
Signed-off-by: Greg Kroah-Hartman <[email protected]>
If you are using a function that does a swap in place,
you cannot just reuse the buffer on the assumption that it has
not been changed.
Signed-off-by: Oliver Neukum <[email protected]>
Signed-off-by: David S. Miller <[email protected]>
When writing to the phy on BE architectures an internal data structure
was directly given, leading to it being byte swapped in the wrong
way for the CPU in 50% of all cases. A temporary buffer must be used.
Signed-off-by: Oliver Neukum <[email protected]>
Signed-off-by: David S. Miller <[email protected]>
If the MTU is large enough, the first write to the device
is just repeated. On BE architectures, however, the first
word of the command will be swapped a second time and garbage
will be written. Avoid that.
Signed-off-by: Oliver Neukum <[email protected]>
Signed-off-by: David S. Miller <[email protected]>
This is the UAS version of
747668dbc0
usb-storage: Set virt_boundary_mask to avoid SG overflows
We are not as likely to be vulnerable as storage, as it is unlikelier
that UAS is run over a controller without native support for SG,
but the issue exists.
The issue has been existing since the inception of the driver.
Fixes: 115bb1ffa5 ("USB: Add UAS driver")
Signed-off-by: Oliver Neukum <[email protected]>
Cc: stable <[email protected]>
Signed-off-by: Greg Kroah-Hartman <[email protected]>
If a device splits up a control message and a reset() happens
between the parts, the message is lost and already recieved parts
must be dropped.
Signed-off-by: Oliver Neukum <[email protected]>
Fixes: 1aba579f3c ("cdc-acm: handle read pipe errors")
Cc: stable <[email protected]>
Signed-off-by: Greg Kroah-Hartman <[email protected]>
If we filter flags before they reach the core we need to generate our
own warnings.
Signed-off-by: Oliver Neukum <[email protected]>
Fixes: 0cb54a3e47 ("USB: debugging code shouldn't alter control flow")
Cc: stable <[email protected]>
Signed-off-by: Greg Kroah-Hartman <[email protected]>
Requesting a ZERO_PACKET or not is sensible only for output.
In the input direction the device decides.
Likewise accepting short packets makes sense only for input.
This allows operation with panic_on_warn without opening up
a local DOS.
Signed-off-by: Oliver Neukum <[email protected]>
Reported-by: [email protected]
Fixes: 0cb54a3e47 ("USB: debugging code shouldn't alter control flow")
Cc: stable <[email protected]>
Signed-off-by: Greg Kroah-Hartman <[email protected]>
This reverts commit a81cf9799a.
The patch causes a regression, which I cannot find the reason for.
So let's revert for now, as a revert hurts only performance.
Original report:
I was trying to resolve the problem with Oliver but we don't get any conclusion
for 5 months, so I am now sending this to mail list and cdc_acm authors.
I am using simple request-response protocol to obtain the boiller parameters
in constant intervals.
A simple one transaction is:
1. opening the /dev/ttyACM0
2. sending the following 10-bytes request to the device:
unsigned char req[] = {0x02, 0xfe, 0x01, 0x05, 0x08, 0x02, 0x01, 0x69, 0xab, 0x03};
3. reading response (frame of 74 bytes length).
4. closing the descriptor
I am doing this transaction with 5 seconds intervals.
Before the bad commit everything was working correctly: I've got a requests and
a responses in a timely manner.
After the bad commit more time I am using the kernel module, more problems I have.
The graph [2] is showing the problem.
As you can see after module load all seems fine but after about 30 minutes I've got
a plenty of EAGAINs when doing read()'s and trying to read back the data.
When I rmmod and insmod the cdc_acm module again, then the situation is starting
over again: running ok shortly after load, and more time it is running, more EAGAINs
I have when calling read().
As a bonus I can see the problem on the device itself:
The device is configured as you can see here on this screen [3].
It has two transmision LEDs: TX and RX. Blink duration is set for 100ms.
This is a recording before the bad commit when all is working fine: [4]
And this is with the bad commit: [5]
As you can see the TX led is blinking wrongly long (indicating transmission?)
and I have problems doing read() calls (EAGAIN).
Reported-by: Mariusz Bialonczyk <[email protected]>
Signed-off-by: Oliver Neukum <[email protected]>
Fixes: a81cf9799a ("cdc-acm: implement put_char() and flush_chars()")
Cc: stable <[email protected]>
Signed-off-by: Greg Kroah-Hartman <[email protected]>
The hope that UAS devices would be less broken than old style storage
devices has turned out to be unfounded. Make UAS support more of the
quirk flags of the old driver.
Signed-off-by: Oliver Neukum <[email protected]>
Cc: stable <[email protected]>
Signed-off-by: Greg Kroah-Hartman <[email protected]>
Quoting the original report:
It looks like there is a double-free vulnerability in Linux usbtv driver
on an error path of usbtv_probe function. When audio registration fails,
usbtv_video_free function ends up freeing usbtv data structure, which
gets freed the second time under usbtv_video_fail label.
usbtv_audio_fail:
usbtv_video_free(usbtv); =>
v4l2_device_put(&usbtv->v4l2_dev);
=> v4l2_device_put
=> kref_put
=> v4l2_device_release
=> usbtv_release (CALLBACK)
=> kfree(usbtv) (1st time)
usbtv_video_fail:
usb_set_intfdata(intf, NULL);
usb_put_dev(usbtv->udev);
kfree(usbtv); (2nd time)
So, as we have refcounting, use it
Reported-by: Yavuz, Tuba <[email protected]>
Signed-off-by: Oliver Neukum <[email protected]>
CC: [email protected]
Signed-off-by: Hans Verkuil <[email protected]>
Signed-off-by: Mauro Carvalho Chehab <[email protected]>
That a kevent could not be scheduled is not an error.
Such handlers must be able to deal with multiple events anyway.
As the successful scheduling of a work is a debug event, make
the failure debug priority, too.
V2: coding style
Signed-off-by: Oliver Neukum <[email protected]>
Reported-by: Cristian Caravena <[email protected]>
Signed-off-by: David S. Miller <[email protected]>
Quoting Hans:
If we return 1 from our post_reset handler, then our disconnect handler
will be called immediately afterwards. Since pre_reset blocks all scsi
requests our disconnect handler will then hang in the scsi_remove_host
call.
This is esp. bad because our disconnect handler hanging for ever also
stops the USB subsys from enumerating any new USB devices, causes commands
like lsusb to hang, etc.
In practice this happens when unplugging some uas devices because the hub
code may see the device as needing a warm-reset and calls usb_reset_device
before seeing the disconnect. In this case uas_configure_endpoints fails
with -ENODEV. We do not want to print an error for this, so this commit
also silences the shost_printk for -ENODEV.
ENDQUOTE
However, if we do that we better drop any unconditional execution
and report to the SCSI subsystem that we have undergone a reset
but we are not operational now.
Signed-off-by: Oliver Neukum <[email protected]>
Reported-by: Hans de Goede <[email protected]>
CC: [email protected]
Signed-off-by: Greg Kroah-Hartman <[email protected]>
This modem needs this quirk to operate. It produces timeouts when
resumed without reset.
Signed-off-by: Oliver Neukum <[email protected]>
CC: [email protected]
Signed-off-by: Greg Kroah-Hartman <[email protected]>
Getting the device version out of the driver really aids debugging.
Signed-off-by: Oliver Neukum <[email protected]>
Signed-off-by: David S. Miller <[email protected]>
Some devices need their multicast filter reset but others are crashed by that.
So the methods need to be separated.
Signed-off-by: Oliver Neukum <[email protected]>
Reported-by: "Ridgway, Keith" <[email protected]>
Signed-off-by: David S. Miller <[email protected]>
The gadget code exports the bitfield for serial status changes
over the wire in its internal endianness. The fix is to convert
to little endian before sending it over the wire.
Signed-off-by: Oliver Neukum <[email protected]>
Tested-by: 家瑋 <[email protected]>
CC: <[email protected]>
Signed-off-by: Felipe Balbi <[email protected]>
The gadget code exports the bitfield for serial status changes
over the wire in its internal endianness. The fix is to convert
to little endian before sending it over the wire.
Signed-off-by: Oliver Neukum <[email protected]>
Tested-by: 家瑋 <[email protected]>
CC: [email protected]
Signed-off-by: Greg Kroah-Hartman <[email protected]>
There is a small window during which the an URB may
remain active after disconnect has returned. If in that case
already freed memory may be accessed and executed.
The fix is to poison the URB befotre the work is flushed.
Signed-off-by: Oliver Neukum <[email protected]>
Signed-off-by: Greg Kroah-Hartman <[email protected]>
This device gives the following error on detection.
xhci_hcd 0000:00:11.0: ERROR Transfer event for disabled endpoint or
incorrect stream ring
The same error is not seen when it is added to unusual_device
list with US_FL_NO_REPORT_OPCODES passed.
Signed-off-by: George Cherian <[email protected]>
Signed-off-by: Oliver Neukum <[email protected]>
CC: [email protected]
Signed-off-by: Greg Kroah-Hartman <[email protected]>
This is a conversion of the USB documentation to the Sphinx format.
No content was altered or reformatted.
Signed-off-by: Oliver <[email protected]>
Signed-off-by: Jonathan Corbet <[email protected]>
variable struct usb_cdc_parsed_header h may be used
uninitialized in acm_probe.
In kernel 4.8.
/* handle quirks deadly to normal probing*/
if (quirks == NO_UNION_NORMAL)
...
goto skip_normal_probe;
}
we bypass call to
cdc_parse_cdc_header(&h, intf, buffer, buflen);
but later use h in
if (h.usb_cdc_country_functional_desc) { /* export the country data */
Signed-off-by: Oliver Neukum <[email protected]>
CC: [email protected]
Reported-by: Victor Sologoubov <[email protected]>
Signed-off-by: Greg Kroah-Hartman <[email protected]>
Like many similar devices it needs a quirk to work.
Issuing the request gets the device into an irrecoverable state.
Signed-off-by: Oliver Neukum <[email protected]>
CC: [email protected]
Signed-off-by: Jiri Kosina <[email protected]>
It does no good to mention The 2.4 kernel series and neglect
USB 3.x and XHCI. Also with type C and micro/mini USB we better
not talk about the shape of connectors.
Signed-off-by: Oliver Neukum <[email protected]>
Acked-by: Greg Kroah-Hartman <[email protected]>
Signed-off-by: Jonathan Corbet <[email protected]>
This should fix the last holes against malicious devices
still open in cdc-acm. It cannot go into stable due to
the introduction of the common parser.
The fix for stable already merged also covers the problems this patch
fixes.
Signed-off-by: Oliver Neukum <[email protected]>
Signed-off-by: Greg Kroah-Hartman <[email protected]>
Further cleanup making the debug messages more precise, useful
and removing mere trace points.
Signed-off-by: Oliver Neukum <[email protected]>
Signed-off-by: Greg Kroah-Hartman <[email protected]>
Some SATA to USB bridges fail to cooperate with some
drives resulting in no cache being present being reported
to the host. That causes the host to skip sending
a command to synchronize caches. That causes data loss
when the drive is powered down.
Signed-off-by: Oliver Neukum <[email protected]>
Reviewed-by: Martin K. Petersen <[email protected]>
Acked-by: Alan Stern <[email protected]>
Signed-off-by: Greg Kroah-Hartman <[email protected]>
Some debug messages merely provide a function trace without
additional debug data. They predate ftrace and can be replaced
by it. Drop them without replacement.
Signed-off-by: Oliver Neukum <[email protected]>
Signed-off-by: Greg Kroah-Hartman <[email protected]>
SOme statements in the driver only served to inform
which functions were entered. Ftrace can do that just as good without
needing memory. Remove the statements.
Signed-off-by: Oliver Neukum <[email protected]>
Signed-off-by: David S. Miller <[email protected]>
This fixes the oops discovered by the Umap2 project and Alan Stern.
The intf member needs to be set before the firmware is downloaded.
Signed-off-by: Oliver Neukum <[email protected]>
Signed-off-by: David S. Miller <[email protected]>
Dynamic debugging will already add the function (and the line number)
to a debug message if one requests that. It makes no sense to add
them unconditionally in a driver.
Signed-off-by: Oliver Neukum <[email protected]>
Signed-off-by: Greg Kroah-Hartman <[email protected]>
This removes some overly long lines by renaming variables and giving
them local scope.
Signed-off-by: Oliver Neukum <[email protected]>
Signed-off-by: Greg Kroah-Hartman <[email protected]>
Now that the common parser resides in USB core, it can
be used for CDC-WDM.
Signed-off-by: Oliver Neukum <[email protected]>
Signed-off-by: Greg Kroah-Hartman <[email protected]>
A small update to unify error handling during probe().
Signed-off-by: Oliver Neukum <[email protected]>
Signed-off-by: Greg Kroah-Hartman <[email protected]>
This introduces the common parser for extra CDC headers now that it no longer
depends on usbnet.
Signed-off-by: Oliver Neukum <[email protected]>
Signed-off-by: Greg Kroah-Hartman <[email protected]>
The dependencies were impossible to handle preventing
drivers for CDC devices not which are not network drivers
from using the common parser.
Signed-off-by: Oliver Neukum <[email protected]>
Signed-off-by: Greg Kroah-Hartman <[email protected]>
This patch turns status in a variable read once from the URB.
The long term plan is to deliver status to the callback.
In addition it makes the code a bit more elegant.
Signed-off-by: Oliver Neukum <[email protected]>
Signed-off-by: Johan Hovold <[email protected]>
Flushing a work that reschedules itself is not a sensible operation. It needs
to be killed. Failure to do so leads to a kernel panic in the timer code.
CC: [email protected]
Signed-off-by: Oliver Neukum <[email protected]>
Reviewed-by: Benjamin Tissoires <[email protected]>
Signed-off-by: Jiri Kosina <[email protected]>
If a port can do 10 Gb/s the kernel should say so.
The corresponding check needs to be added.
Signed-off.by: Oliver Neukum <[email protected]>>
Signed-off-by: Greg Kroah-Hartman <[email protected]>
SS+ also expresses intervals in units of 125ms. Testing must
be for SS or faster, not SS exactly.
Signed-off-by: Oliver neukum <[email protected]>
Signed-off-by: Greg Kroah-Hartman <[email protected]>
LTM is also defined for SS+. The correct test is to check for anything
slower than SS not exactly SS.
Signed-off-by: Oliver Neukum <[email protected]>
Signed-off-by: Greg Kroah-Hartman <[email protected]>
This takes the definitions of requests from chapter 9.3.1
of the USB Power Delivery spec.
Signed-off-by: Oliver Neukum <[email protected]>
Signed-off-by: Greg Kroah-Hartman <[email protected]>
Adding the descriptors of chapter 9.2 of the Power Delivery spec.
Signed-off-by: Oliver Neukum <[email protected]>
Signed-off-by: Greg Kroah-Hartman <[email protected]>
Under some circumstances acm_tty_flush_chars() is called
with no buffer to flush. We simply need to do nothing.
Signed-off-by: Oliver Neukum <[email protected]>
Reported-by: Torsten Hilbrich <[email protected]>
Signed-off-by: Greg Kroah-Hartman <[email protected]>
The device can now easily be derived from the interface.
Stop leaving a private copy.
Signed-off-by: Oliver Neukum <[email protected]>
Signed-off-by: Dmitry Torokhov <[email protected]>
The device can now easily be derived from the interface.
Stop leaving a private copy.
Signed-off-by: Oliver Neukum <[email protected]>
Signed-off-by: Dmitry Torokhov <[email protected]>
The device can now easily be derived from the interface.
Stop leaving a private copy.
Signed-off-by: Oliver Neukum <[email protected]>
Signed-off-by: Dmitry Torokhov <[email protected]>
The device can now easily be derived from the interface.
Stop leaving a private copy.
Signed-off-by: Oliver Neukum <[email protected]>
Signed-off-by: Dmitry Torokhov <[email protected]>
An attack using the lack of sanity checking in probe is known. This
patch checks for the existence of a second port.
CVE-2016-3136
Signed-off-by: Oliver Neukum <[email protected]>
CC: [email protected]
[johan: add error message ]
Signed-off-by: Johan Hovold <[email protected]>
Signed-off-by: Greg Kroah-Hartman <[email protected]>
During the initialisation the driver uses a buffer on the stack for DMA.
That violates the cache coherency rules. The fix is to allocate the buffer
with kmalloc().
Signed-off-by: Oliver Neukum <[email protected]>
Signed-off-by: Dmitry Torokhov <[email protected]>
An attack has become available which pretends to be a quirky
device circumventing normal sanity checks and crashes the kernel
by an insufficient number of interfaces. This patch adds a check
to the code path for quirky devices.
Signed-off-by: Oliver Neukum <[email protected]>
CC: [email protected]
Signed-off-by: Greg Kroah-Hartman <[email protected]>
Attacks that trick drivers into passing a NULL pointer
to usb_driver_claim_interface() using forged descriptors are
known. This thwarts them by sanity checking.
Signed-off-by: Oliver Neukum <[email protected]>
CC: [email protected]
Signed-off-by: Greg Kroah-Hartman <[email protected]>
In case bind() works, but a later error forces bailing
in probe() in error cases work and a timer may be scheduled.
They must be killed. This fixes an error case related to
the double free reported in
http://www.spinics.net/lists/netdev/msg367669.html
and needs to go on top of Linus' fix to cdc-ncm.
Signed-off-by: Oliver Neukum <[email protected]>
Signed-off-by: David S. Miller <[email protected]>
A typo of j for i led to a logic bug. To rule out future
confusion, the variable names are made meaningful.
Signed-off-by: Oliver Neukum <[email protected]>
CC: [email protected]
Signed-off-by: Greg Kroah-Hartman <[email protected]>
To allow for and clean handling of signals an URB is introduced.
Signed-off-by: Oliver Neukum <[email protected]>
Signed-off-by: Keith Packard <[email protected]>
Signed-off-by: Greg Kroah-Hartman <[email protected]>
Shares the cleanup code between all probe failure paths, instead of
having per-failure cleanup at each point in the function.
Signed-off-by: Oliver Neukum <[email protected]>
Signed-off-by: Keith Packard <[email protected]>
Signed-off-by: Greg Kroah-Hartman <[email protected]>
Some devices I got show an inability to operate right after
power on if they are already connected. They are beyond recovery
if the descriptors are requested multiple times. So in case of
a timeout we rather bail early and reset again. But it must be
done only on the first loop lest we get into a reset/time out
spiral that can be overcome with a retry.
This patch is a rework of a patch that fell through the cracks.
http://www.spinics.net/lists/linux-usb/msg103263.html
Signed-off-by: Oliver Neukum <[email protected]>
CC: [email protected]
Signed-off-by: Greg Kroah-Hartman <[email protected]>
This should cut down latencies and waste if the tty layer writes single bytes.
Signed-off-by: Oliver Neukum >[email protected]>
Signed-off-by: Greg Kroah-Hartman <[email protected]>
Quting the relevant thread:
> In fact, I suspect the locking added by the kernel 3.13 commit for
> read_descriptors() is invalid because read_descriptors() performs no USB
> activity; read_descriptors() just reads information from an allocated
> memory structure. This structure is protected as the structure is
> existing before and after the sysfs vfs descriptors entry is created or
> destroyed.
You're right. For some reason I thought that usb_deauthorize_device()
would destroy the rawdescriptor structures (as mentioned in that
commit's Changelog), but it doesn't. The locking in read_descriptors()
is unnecessary.
> The information is only written at the time of enumeration
> and does not change. At least that is my understanding.
>
> It is noted that in our testing of kernel 3.8 on ARM, that sysfs
> read_descriptors() was non-blocking because the kernel 3.13 comment was
> not there.
>
> The pre-kernel 3.13 sysfs read_descriptors() seemed to work OK.
>
> Proposal:
> =========
>
> Remove the usb_lock_device(udev) and usb_unlock_device(udev) from
> devices/usb/core/sysfs.c in read_descriptors() that was added by the
> kernel 3.13 commit
> "232275a USB: fix substandard locking for the sysfs files"
>
> Any comments to this proposal ?
It seems okay to me. Please submit a patch.
So this removes the locking making the point about -EINTR in
the first path moot.
Signed-off-by: Oliver Neukum <[email protected]>
Signed-off-by: Greg Kroah-Hartman <[email protected]>
232275a USB: fix substandard locking for the sysfs files
introduced needed locking into sysfs operations on USB devices
It, however, uses uninterruptible sleep and if the error
handling is on extreme cases of sleep lengths of 10s of seconds
are possible. Unless we are removing the device we should use
interruptible sleep.
Signed-off-by: Oliver Neukum <[email protected]>
Signed-off-by: Greg Kroah-Hartman <[email protected]>
This phone needs to be handled by a specialised firmware tool
and is reported to crash irrevocably if cdc-acm takes it.
Signed-off-by: Oliver Neukum <[email protected]>
CC: [email protected]
Signed-off-by: Greg Kroah-Hartman <[email protected]>
Oliver Neukum