From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752883AbcELKRC (ORCPT ); Thu, 12 May 2016 06:17:02 -0400 Received: from mga03.intel.com ([134.134.136.65]:2274 "EHLO mga03.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751336AbcELKQ7 (ORCPT ); Thu, 12 May 2016 06:16:59 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.24,609,1455004800"; d="asc'?scan'208";a="101871348" From: Felipe Balbi To: "Du\, Changbin" , Al Viro Cc: "gregkh\@linuxfoundation.org" , "mina86\@mina86.com" , "rui.silva\@linaro.org" , "k.opasiak\@samsung.com" , "lars\@metafoo.de" , "linux-usb\@vger.kernel.org" , "linux-kernel\@vger.kernel.org" Subject: RE: [PATCH] usb: gadget: f_fs: report error if excess data received In-Reply-To: <0C18FE92A7765D4EB9EE5D38D86A563A05D2F2C8@SHSMSX103.ccr.corp.intel.com> References: <1462961970-2001-1-git-send-email-changbin.du@intel.com> <87twi4g8s2.fsf@linux.intel.com> <0C18FE92A7765D4EB9EE5D38D86A563A05D2F01F@SHSMSX103.ccr.corp.intel.com> <87a8jvg43q.fsf@linux.intel.com> <0C18FE92A7765D4EB9EE5D38D86A563A05D2F156@SHSMSX103.ccr.corp.intel.com> <874ma3g1lq.fsf@linux.intel.com> <0C18FE92A7765D4EB9EE5D38D86A563A05D2F183@SHSMSX103.ccr.corp.intel.com> <87zirveixx.fsf@linux.intel.com> <0C18FE92A7765D4EB9EE5D38D86A563A05D2F2C8@SHSMSX103.ccr.corp.intel.com> User-Agent: Notmuch/0.22+11~g124a67e (http://notmuchmail.org) Emacs/25.0.93.2 (x86_64-pc-linux-gnu) Date: Thu, 12 May 2016 13:13:47 +0300 Message-ID: <87twi3eg84.fsf@linux.intel.com> MIME-Version: 1.0 Content-Type: multipart/signed; boundary="=-=-="; Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org protocol="application/pgp-signature" --=-=-= Content-Type: text/plain Content-Transfer-Encoding: quoted-printable Hi, "Du, Changbin" writes: >> >> > These all can lead host send more than device wanted bytes. For sure >> >> > it wrong at host side, but device side don't know. >> >> >> >> but none of this means we have a bug at device side. In fact, by >> >> allowing these extra bytes to reach userspace, we could be creating a >> >> possible attack vector. >> >> >> >> Your explanation is unsatisfactory, so I won't apply your patch, sorr= y. >> >> >> >> -- >> >> balbi >> > It is fine. Then need userspace take care of all the data it received.= Because >> > Kernel may drop some data for it. Kernel ffs driver is unauthentic >> sometimes. >>=20 >> I really cannot understand what you mean sometimes. You're saying that >> userspace needs to take care of all the data it received because kernel >> can drop data. If kernel is dropping data, there's no extra data >> reaching userspace, right? >>=20 > For sure, maybe I didn't describe it well so let you confused. :) okay >> Is the problem that we *are* giving more data than expected to >> userspace? Are we overflowing some userspace buffer? If that's the case, >> then below should be enough for the time being: >>=20 > No, the problem is we drop data but silently. We cannot give more data to okay, but does that create any problems for device side userspace? What problem is that? > userspace since buffer is limited. right, and that was my point: if we copy more to userspace, then we have a real big problem. >> @@ -811,7 +815,12 @@ static ssize_t ffs_epfile_io(struct file *file, str= uct >> ffs_io_data *io_data) >> */ >> ret =3D interrupted ? -EINTR : ep->status; >> if (io_data->read && ret > 0) { >> - ret =3D copy_to_iter(data, ret, &io_data->data); >> + if (ret > io_data->expected_len) >> + pr_debug("FFS: size mismatch: %zd for %zd", >> + ret, io_data->expected_len); >> + >> + ret =3D copy_to_iter(data, io_data->expected_len, >> + &io_data->data); >> if (!ret) >> ret =3D -EFAULT; >> } >>=20 >> that we can get merged during v4.7-rc and Cc stable and backport this to >> anything containing Al's commit c993c39b8639 ("gadget/function/f_fs.c: >> use put iov_iter into io_data"). >>=20 > > The different for this code is just give warning but not return > error. It is also fine for me that at least this let development can > find some key message to find What happed under kernel. But the > message should be *error* I think. I'm fine with pr_error() > And this missed AIO path. This is identify to my patch after remove the right, it's more of a debug patch since I don't have the setup to trigger this (I'm assuming you're using adb?) > "return -EOVERFLOW;" line. there's one key difference, see below > Byw, we not need add the field "expected_len", we can get it from the > struct ffs_io_data. without expected_len we can copy more data to userspace, right ? If req->actual > data_len_before_aligning_to_maxpacket, then we will copy more data then we should to userspace and this was a regression caused by Al's commit, AFAICT. > If this is fine for you, I can publish a new patch. > >> -- >> Balbi > > Best Regards, > Du, Changbin =2D-=20 balbi --=-=-= Content-Type: application/pgp-signature; name="signature.asc" --=-=-=--