Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Fri, 31 Jan 2020 14:30:04 +0100
From:   Johan Hovold <johan@kernel.org>
To:     Dan Carpenter <dan.carpenter@oracle.com>
Cc:     gregkh@linuxfoundation.org, Alan Stern <stern@rowland.harvard.edu>,
        syzbot <syzbot+1bc2c2afd44f820a669f@syzkaller.appspotmail.com>,
        andreyknvl@google.com, ingrassia@epigenesys.com,
        linux-kernel@vger.kernel.org, linux-usb@vger.kernel.org,
        syzkaller-bugs@googlegroups.com
Subject: Re: [PATCH] usb: core: urb: change a dev_WARN() to dev_err() for
 syzbot
Message-ID: <20200131133004.GD10381@localhost>
References: <00000000000095e1d8059d4675ac@google.com>
 <20200131050651.hlq27kehtir3agf2@kili.mountain>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200131050651.hlq27kehtir3agf2@kili.mountain>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Fri, Jan 31, 2020 at 08:06:52AM +0300, Dan Carpenter wrote:
> We changed this from dev_err() to dev_WARN() in commit 0cb54a3e47cb
> ("USB: debugging code shouldn't alter control flow").
>
> The difference between dev_WARN() and dev_err() is that dev_WARN()
> prints a stack trace and if you have panic on OOPS enabled then it leads
> to a panic.  The dev_err() function just prints the error message.
> 
> Back in the day we didn't have usb emulators fuzz testing the kernel
> so dev_WARN() didn't cause a problem for anyone, but these days the
> dev_WARN() interferes with syzbot so let's change this to a dev_err().

The commit you refer to did more than just change dev_err() to
dev_WARN(); it also stopped returning an error in case a driver
submitted an URB for an endpoint of the wrong type. At that point in
time all this was dependent on CONFIG_USB_DEBUG however.

> Reported-by: syzbot+1bc2c2afd44f820a669f@syzkaller.appspotmail.com
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> 
>  drivers/usb/core/urb.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/core/urb.c b/drivers/usb/core/urb.c
> index da923ec17612..0980c1d2253d 100644
> --- a/drivers/usb/core/urb.c
> +++ b/drivers/usb/core/urb.c
> @@ -475,7 +475,7 @@ int usb_submit_urb(struct urb *urb, gfp_t mem_flags)
>  
>  	/* Check that the pipe's type matches the endpoint's type */
>  	if (usb_urb_ep_type_check(urb))
> -		dev_WARN(&dev->dev, "BOGUS urb xfer, pipe %x != type %x\n",
> +		dev_err(&dev->dev, "BOGUS urb xfer, pipe %x != type %x\n",
>  			usb_pipetype(urb->pipe), pipetypes[xfertype]);
>  
>  	/* Check against a simple/standard policy */

It seems this change would just be papering over these driver bugs. The
dev_WARN() is there in the first place to allow us to catch them.

Even if it takes some work, it should be doable to track down and add
the missing sanity checks to the drivers that lack them. Some have
already been fixed, and I have some more pending patches to fix or add
helpers to simplify fixing the remaining ones.

Johan