Re: drivers/usb/core/devio.c:1155 do_proc_control() error: copy_from_user() 'tbuf' too small (4096 vs 8192)

[Dan Carpenter]

On Mon, Aug 10, 2020 at 02:14:33PM +0200, Christoph Hellwig wrote:
> On Mon, Aug 10, 2020 at 02:12:23PM +0200, Christoph Hellwig wrote:
> > As far as I can tell the warning is valid as we copy a user controlled
> > amount into a fixed sized buffer.  But this an old condition not actually
> > created by this commit..
> 
> Actually, is is not.  do_proc_control checks the legnth, but does it
> using a somewhat pointless local variable, which might have confused
> smatch.

I think what's happening is that this is using PAGE_SIZE = 8096 and
smatch assumes that it's always 4096.  It seems obvious in retrospect
that you can't assume page size if 4096 but I'm not sure how to
calculate the actual page size...

Normally I filter all Smatch kbuild bot warnings manually before
forwarding them but this one was sent directly.

regards,
dan carpenter

> 
> > 
> > On Sat, Aug 08, 2020 at 01:26:39PM +0800, kernel test robot wrote:
> > > tree:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
> > > head:   049eb096da48db0421dd5e358b9b082a1a8a2025
> > > commit: c17536d0abde2fd24afca542e3bb73b45a299633 usb: usbfs: stop using compat_alloc_user_space
> > > date:   2 weeks ago
> > > config: nds32-randconfig-m031-20200808 (attached as .config)
> > > compiler: nds32le-linux-gcc (GCC) 9.3.0
> > > 
> > > If you fix the issue, kindly add following tag as appropriate
> > > Reported-by: kernel test robot <lkp@xxxxxxxxx>
> > > 
> > > smatch warnings:
> > > drivers/usb/core/devio.c:1155 do_proc_control() error: copy_from_user() 'tbuf' too small (4096 vs 8192)
> > > 
> > > vim +/tbuf +1155 drivers/usb/core/devio.c
> > > 
> > >   1104	
> > >   1105	static int do_proc_control(struct usb_dev_state *ps,
> > >   1106			struct usbdevfs_ctrltransfer *ctrl)
> > >   1107	{
> > >   1108		struct usb_device *dev = ps->dev;
> > >   1109		unsigned int tmo;
> > >   1110		unsigned char *tbuf;
> > >   1111		unsigned wLength;
> > >   1112		int i, pipe, ret;
> > >   1113	
> > >   1114		ret = check_ctrlrecip(ps, ctrl->bRequestType, ctrl->bRequest,
> > >   1115				      ctrl->wIndex);
> > >   1116		if (ret)
> > >   1117			return ret;
> > >   1118		wLength = ctrl->wLength;	/* To suppress 64k PAGE_SIZE warning */
> > >   1119		if (wLength > PAGE_SIZE)
> > >   1120			return -EINVAL;
> > >   1121		ret = usbfs_increase_memory_usage(PAGE_SIZE + sizeof(struct urb) +
> > >   1122				sizeof(struct usb_ctrlrequest));
> > >   1123		if (ret)
> > >   1124			return ret;
> > >   1125		tbuf = (unsigned char *)__get_free_page(GFP_KERNEL);
> > >   1126		if (!tbuf) {
> > >   1127			ret = -ENOMEM;
> > >   1128			goto done;
> > >   1129		}
> > >   1130		tmo = ctrl->timeout;
> > >   1131		snoop(&dev->dev, "control urb: bRequestType=%02x "
> > >   1132			"bRequest=%02x wValue=%04x "
> > >   1133			"wIndex=%04x wLength=%04x\n",
> > >   1134			ctrl->bRequestType, ctrl->bRequest, ctrl->wValue,
> > >   1135			ctrl->wIndex, ctrl->wLength);
> > >   1136		if (ctrl->bRequestType & 0x80) {
> > >   1137			pipe = usb_rcvctrlpipe(dev, 0);
> > >   1138			snoop_urb(dev, NULL, pipe, ctrl->wLength, tmo, SUBMIT, NULL, 0);
> > >   1139	
> > >   1140			usb_unlock_device(dev);
> > >   1141			i = usb_control_msg(dev, pipe, ctrl->bRequest,
> > >   1142					    ctrl->bRequestType, ctrl->wValue, ctrl->wIndex,
> > >   1143					    tbuf, ctrl->wLength, tmo);
> > >   1144			usb_lock_device(dev);
> > >   1145			snoop_urb(dev, NULL, pipe, max(i, 0), min(i, 0), COMPLETE,
> > >   1146				  tbuf, max(i, 0));
> > >   1147			if ((i > 0) && ctrl->wLength) {
> > >   1148				if (copy_to_user(ctrl->data, tbuf, i)) {
> > >   1149					ret = -EFAULT;
> > >   1150					goto done;
> > >   1151				}
> > >   1152			}
> > >   1153		} else {
> > >   1154			if (ctrl->wLength) {
> > > > 1155				if (copy_from_user(tbuf, ctrl->data, ctrl->wLength)) {
> > >   1156					ret = -EFAULT;
> > >   1157					goto done;
> > >   1158				}
> > >   1159			}
> > >   1160			pipe = usb_sndctrlpipe(dev, 0);
> > >   1161			snoop_urb(dev, NULL, pipe, ctrl->wLength, tmo, SUBMIT,
> > >   1162				tbuf, ctrl->wLength);
> > >   1163	
> > >   1164			usb_unlock_device(dev);
> > >   1165			i = usb_control_msg(dev, usb_sndctrlpipe(dev, 0), ctrl->bRequest,
> > >   1166					    ctrl->bRequestType, ctrl->wValue, ctrl->wIndex,
> > >   1167					    tbuf, ctrl->wLength, tmo);
> > >   1168			usb_lock_device(dev);
> > >   1169			snoop_urb(dev, NULL, pipe, max(i, 0), min(i, 0), COMPLETE, NULL, 0);
> > >   1170		}
> > >   1171		if (i < 0 && i != -EPIPE) {
> > >   1172			dev_printk(KERN_DEBUG, &dev->dev, "usbfs: USBDEVFS_CONTROL "
> > >   1173				   "failed cmd %s rqt %u rq %u len %u ret %d\n",
> > >   1174				   current->comm, ctrl->bRequestType, ctrl->bRequest,
> > >   1175				   ctrl->wLength, i);
> > >   1176		}
> > >   1177		ret = i;
> > >   1178	 done:
> > >   1179		free_page((unsigned long) tbuf);
> > >   1180		usbfs_decrease_memory_usage(PAGE_SIZE + sizeof(struct urb) +
> > >   1181				sizeof(struct usb_ctrlrequest));
> > >   1182		return ret;
> > >   1183	}
> > >   1184	
> > > 
> > > ---
> > > 0-DAY CI Kernel Test Service, Intel Corporation
> > > https://lists.01.org/hyperkitty/list/kbuild-all@xxxxxxxxxxxx
> > 
> > 
> > ---end quoted text---
> ---end quoted text---