linux-kernel - Re: drivers: usb: musb: question about missing break in switch

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Thu, 09 Feb 2017 19:25:26 -0600
From:   "Gustavo A. R. Silva" <garsilva@...eddedor.com>
To:     Bin Liu <b-liu@...com>
Cc:     Greg KH <greg@...ah.com>, linux-usb@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        Peter Senna Tschudin <peter.senna@...il.com>
Subject: Re: drivers: usb: musb: question about missing break in switch

Hello Bin,

Quoting Bin Liu <b-liu@...com>:

> On Thu, Feb 09, 2017 at 02:37:34AM -0600, Gustavo A. R. Silva wrote:
>> Hello everybody,
>>
>> I ran into the following piece of code at
>> drivers/usb/musb/musb_core.c:1854 (linux-next)
>>
>> 1854/*
>> 1855 * Check the musb devctl session bit to determine if we want to
>> 1856 * allow PM runtime for the device. In general, we want to keep things
>> 1857 * active when the session bit is set except after host disconnect.
>> 1858 *
>> 1859 * Only called from musb_irq_work. If this ever needs to get called
>> 1860 * elsewhere, proper locking must be implemented for musb->session.
>> 1861 */
>> 1862static void musb_pm_runtime_check_session(struct musb *musb)
>> 1863{
>> 1864        u8 devctl, s;
>> 1865        int error;
>> 1866
>> 1867        devctl = musb_readb(musb->mregs, MUSB_DEVCTL);
>> 1868
>> 1869        /* Handle session status quirks first */
>> 1870        s = MUSB_DEVCTL_FSDEV | MUSB_DEVCTL_LSDEV |
>> 1871                MUSB_DEVCTL_HR;
>> 1872        switch (devctl & ~s) {
>> 1873        case MUSB_QUIRK_B_INVALID_VBUS_91:
>> 1874                if (musb->quirk_retries--) {
>> 1875                        musb_dbg(musb,
>> 1876                                 "Poll devctl on invalid vbus,
>> assume no session");
>> 1877                        schedule_delayed_work(&musb->irq_work,
>> 1878                                              msecs_to_jiffies(1000));
>> 1879
>> 1880                        return;
>> 1881                }
>> 1882        case MUSB_QUIRK_A_DISCONNECT_19:
>> 1883                if (musb->quirk_retries--) {
>> 1884                        musb_dbg(musb,
>> 1885                                 "Poll devctl on possible host
>> mode disconnect");
>> 1886                        schedule_delayed_work(&musb->irq_work,
>> 1887                                              msecs_to_jiffies(1000));
>> 1888
>> 1889                        return;
>> 1890                }
>> 1891                if (!musb->session)
>> 1892                        break;
>> 1893                musb_dbg(musb, "Allow PM on possible host mode
>> disconnect");
>> 1894                pm_runtime_mark_last_busy(musb->controller);
>> 1895                pm_runtime_put_autosuspend(musb->controller);
>> 1896                musb->session = false;
>> 1897                return;
>> 1898        default:
>> 1899                break;
>> 1900        }
>> 1901
>> 1902        /* No need to do anything if session has not changed */
>> 1903        s = devctl & MUSB_DEVCTL_SESSION;
>> 1904        if (s == musb->session)
>> 1905                return;
>> 1906
>> 1907        /* Block PM or allow PM? */
>> 1908        if (s) {
>> 1909                musb_dbg(musb, "Block PM on active session:
>> %02x", devctl);
>> 1910                error = pm_runtime_get_sync(musb->controller);
>> 1911                if (error < 0)
>> 1912                        dev_err(musb->controller, "Could not
>> enable: %i\n",
>> 1913                                error);
>> 1914                musb->quirk_retries = 3;
>> 1915        } else {
>> 1916                musb_dbg(musb, "Allow PM with no session:  
>> %02x", devctl);
>> 1917                pm_runtime_mark_last_busy(musb->controller);
>> 1918                pm_runtime_put_autosuspend(musb->controller);
>> 1919        }
>> 1920
>> 1921        musb->session = s;
>> 1922}
>>
>> The thing is that the case for MUSB_QUIRK_B_INVALID_VBUS_91 is not
>> terminated by a break statement, and it falls through to the next
>> case MUSB_QUIRK_A_DISCONNECT_19, in case "if
>> (musb->quirk_retries--)" turns to be false.
>>
>> My question here is if this code is intentional?
>
> Yes, it is. For both MUSB_QUIRK_B_INVALID_VBUS_91 and
> MUSB_QUIRK_A_DISCONNECT_19 cases, we first do retries, then if
> musb->session is set, we allow PM.
>

Thanks for clarifying :)
--
Gustavo A. R. Silva