Re: [PATCH] USB: core: limit access to rawdescriptors which were not allocated
On 8/26/20 2:00 AM, Alan Stern wrote:
On Wed, Aug 26, 2020 at 12:16:59AM +0800, yanfei...@windriver.com wrote:
From: Yanfei Xu
When using systemcall to read the rawdescriptors, make sure we won't
access to the rawdescriptors never allocated, which are number
exceed the USB_MAXCONFIG.
Reported-by: syzbot+256e56ddde8b8957e...@syzkaller.appspotmail.com
Signed-off-by: Yanfei Xu
---
drivers/usb/core/sysfs.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/core/sysfs.c b/drivers/usb/core/sysfs.c
index a2ca38e25e0c..1a7a625e5f55 100644
--- a/drivers/usb/core/sysfs.c
+++ b/drivers/usb/core/sysfs.c
@@ -895,7 +895,8 @@ read_descriptors(struct file *filp, struct kobject *kobj,
* configurations (config plus subsidiary descriptors).
*/
for (cfgno = -1; cfgno < udev->descriptor.bNumConfigurations &&
- nleft > 0; ++cfgno) {
+ nleft > 0 &&
+ cfgno < USB_MAXCONFIG; ++cfgno) {
if (cfgno < 0) {
src = >descriptor;
srclen = sizeof(struct usb_device_descriptor);
This is not the right way to fix the problem.
Instead, we should make sure that udev->descriptor.bNumConfigurations is
always <= USB_MAXCONFIG. That's what this code in
usb_get_configuration() is supposed to do:
int ncfg = dev->descriptor.bNumConfigurations;
...
if (ncfg > USB_MAXCONFIG) {
dev_warn(ddev, "too many configurations: %d, "
"using maximum allowed: %d\n", ncfg, USB_MAXCONFIG);
dev->descriptor.bNumConfigurations = ncfg = USB_MAXCONFIG;
}
If you want to fix the bug, you need to figure out why this isn't
working.
Thanks for you suggestion. The patch is not right. I'll try to look
into the root cause.
Regard,
Yanfei.
Alan Stern
Re: [PATCH] USB: core: limit access to rawdescriptors which were not allocated
Hi,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on usb/usb-testing]
[also build test ERROR on balbi-usb/testing/next peter.chen-usb/ci-for-usb-next
v5.9-rc2 next-20200825]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url:
https://github.com/0day-ci/linux/commits/yanfei-xu-windriver-com/USB-core-limit-access-to-rawdescriptors-which-were-not-allocated/20200826-002050
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git
usb-testing
config: arm-randconfig-r015-20200826 (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project
77e5a195f818b9ace91f7b12ab948b21d7918238)
reproduce (this is a W=1 build):
wget
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O
~/bin/make.cross
chmod +x ~/bin/make.cross
# install arm cross compiling tool for clang build
# apt-get install binutils-arm-linux-gnueabi
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=arm
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot
All errors (new ones prefixed by >>):
>> drivers/usb/core/sysfs.c:899:12: error: use of undeclared identifier
>> 'USB_MAXCONFIG'
cfgno < USB_MAXCONFIG; ++cfgno) {
^
1 error generated.
#
https://github.com/0day-ci/linux/commit/dda85cff0852edc4723d1175486a50024ee7289a
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review
yanfei-xu-windriver-com/USB-core-limit-access-to-rawdescriptors-which-were-not-allocated/20200826-002050
git checkout dda85cff0852edc4723d1175486a50024ee7289a
vim +/USB_MAXCONFIG +899 drivers/usb/core/sysfs.c
880
881 static ssize_t
882 read_descriptors(struct file *filp, struct kobject *kobj,
883 struct bin_attribute *attr,
884 char *buf, loff_t off, size_t count)
885 {
886 struct device *dev = kobj_to_dev(kobj);
887 struct usb_device *udev = to_usb_device(dev);
888 size_t nleft = count;
889 size_t srclen, n;
890 int cfgno;
891 void *src;
892
893 /* The binary attribute begins with the device descriptor.
894 * Following that are the raw descriptor entries for all the
895 * configurations (config plus subsidiary descriptors).
896 */
897 for (cfgno = -1; cfgno < udev->descriptor.bNumConfigurations &&
898 nleft > 0 &&
> 899 cfgno < USB_MAXCONFIG; ++cfgno) {
900 if (cfgno < 0) {
901 src = >descriptor;
902 srclen = sizeof(struct usb_device_descriptor);
903 } else {
904 src = udev->rawdescriptors[cfgno];
905 srclen = __le16_to_cpu(udev->config[cfgno].desc.
906 wTotalLength);
907 }
908 if (off < srclen) {
909 n = min(nleft, srclen - (size_t) off);
910 memcpy(buf, src + off, n);
911 nleft -= n;
912 buf += n;
913 off = 0;
914 } else {
915 off -= srclen;
916 }
917 }
918 return count - nleft;
919 }
920
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org
.config.gz
Description: application/gzip
Re: [PATCH] USB: core: limit access to rawdescriptors which were not allocated
Hi,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on usb/usb-testing]
[also build test ERROR on balbi-usb/testing/next peter.chen-usb/ci-for-usb-next
v5.9-rc2 next-20200825]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url:
https://github.com/0day-ci/linux/commits/yanfei-xu-windriver-com/USB-core-limit-access-to-rawdescriptors-which-were-not-allocated/20200826-002050
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git
usb-testing
config: i386-allyesconfig (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
reproduce (this is a W=1 build):
# save the attached .config to linux build tree
make W=1 ARCH=i386
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot
All errors (new ones prefixed by >>):
drivers/usb/core/sysfs.c: In function 'read_descriptors':
>> drivers/usb/core/sysfs.c:899:12: error: 'USB_MAXCONFIG' undeclared (first
>> use in this function); did you mean 'USB_DT_CONFIG'?
899 |cfgno < USB_MAXCONFIG; ++cfgno) {
|^
|USB_DT_CONFIG
drivers/usb/core/sysfs.c:899:12: note: each undeclared identifier is
reported only once for each function it appears in
#
https://github.com/0day-ci/linux/commit/dda85cff0852edc4723d1175486a50024ee7289a
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review
yanfei-xu-windriver-com/USB-core-limit-access-to-rawdescriptors-which-were-not-allocated/20200826-002050
git checkout dda85cff0852edc4723d1175486a50024ee7289a
vim +899 drivers/usb/core/sysfs.c
880
881 static ssize_t
882 read_descriptors(struct file *filp, struct kobject *kobj,
883 struct bin_attribute *attr,
884 char *buf, loff_t off, size_t count)
885 {
886 struct device *dev = kobj_to_dev(kobj);
887 struct usb_device *udev = to_usb_device(dev);
888 size_t nleft = count;
889 size_t srclen, n;
890 int cfgno;
891 void *src;
892
893 /* The binary attribute begins with the device descriptor.
894 * Following that are the raw descriptor entries for all the
895 * configurations (config plus subsidiary descriptors).
896 */
897 for (cfgno = -1; cfgno < udev->descriptor.bNumConfigurations &&
898 nleft > 0 &&
> 899 cfgno < USB_MAXCONFIG; ++cfgno) {
900 if (cfgno < 0) {
901 src = >descriptor;
902 srclen = sizeof(struct usb_device_descriptor);
903 } else {
904 src = udev->rawdescriptors[cfgno];
905 srclen = __le16_to_cpu(udev->config[cfgno].desc.
906 wTotalLength);
907 }
908 if (off < srclen) {
909 n = min(nleft, srclen - (size_t) off);
910 memcpy(buf, src + off, n);
911 nleft -= n;
912 buf += n;
913 off = 0;
914 } else {
915 off -= srclen;
916 }
917 }
918 return count - nleft;
919 }
920
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org
.config.gz
Description: application/gzip
Re: [PATCH] USB: core: limit access to rawdescriptors which were not allocated
On Wed, Aug 26, 2020 at 12:16:59AM +0800, yanfei...@windriver.com wrote:
> From: Yanfei Xu
>
> When using systemcall to read the rawdescriptors, make sure we won't
> access to the rawdescriptors never allocated, which are number
> exceed the USB_MAXCONFIG.
>
> Reported-by: syzbot+256e56ddde8b8957e...@syzkaller.appspotmail.com
> Signed-off-by: Yanfei Xu
> ---
> drivers/usb/core/sysfs.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/core/sysfs.c b/drivers/usb/core/sysfs.c
> index a2ca38e25e0c..1a7a625e5f55 100644
> --- a/drivers/usb/core/sysfs.c
> +++ b/drivers/usb/core/sysfs.c
> @@ -895,7 +895,8 @@ read_descriptors(struct file *filp, struct kobject *kobj,
>* configurations (config plus subsidiary descriptors).
>*/
> for (cfgno = -1; cfgno < udev->descriptor.bNumConfigurations &&
> - nleft > 0; ++cfgno) {
> + nleft > 0 &&
> + cfgno < USB_MAXCONFIG; ++cfgno) {
> if (cfgno < 0) {
> src = >descriptor;
> srclen = sizeof(struct usb_device_descriptor);
This is not the right way to fix the problem.
Instead, we should make sure that udev->descriptor.bNumConfigurations is
always <= USB_MAXCONFIG. That's what this code in
usb_get_configuration() is supposed to do:
int ncfg = dev->descriptor.bNumConfigurations;
...
if (ncfg > USB_MAXCONFIG) {
dev_warn(ddev, "too many configurations: %d, "
"using maximum allowed: %d\n", ncfg, USB_MAXCONFIG);
dev->descriptor.bNumConfigurations = ncfg = USB_MAXCONFIG;
}
If you want to fix the bug, you need to figure out why this isn't
working.
Alan Stern
