tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
head: 2caeeb9d4a1bccd923b7918427f9e9ef7151ddd8
commit: c5c354a3a4728045e1342166394c615d75d45377 drm/udl: Fix inconsistent urbs.count value during udl_free_urb_list()
config: parisc-randconfig-m031-20230421 (https://download.01.org/0day-ci/archive/20230423/[email protected]/config)
compiler: hppa-linux-gcc (GCC) 12.1.0
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
| Reported-by: Dan Carpenter <[email protected]>
| Link: https://lore.kernel.org/r/[email protected]/
smatch warnings:
drivers/gpu/drm/udl/udl_main.c:259 udl_get_urb_locked() warn: can 'unode' even be NULL?
vim +/unode +259 drivers/gpu/drm/udl/udl_main.c
c5c354a3a47280 Takashi Iwai 2022-09-08 236 static struct urb *udl_get_urb_locked(struct udl_device *udl, long timeout)
5320918b9a8786 Dave Airlie 2010-12-15 237 {
c5c354a3a47280 Takashi Iwai 2022-09-08 238 struct urb_node *unode;
5320918b9a8786 Dave Airlie 2010-12-15 239
c5c354a3a47280 Takashi Iwai 2022-09-08 240 assert_spin_locked(&udl->urbs.lock);
5320918b9a8786 Dave Airlie 2010-12-15 241
acd45c56790a3b Takashi Iwai 2022-08-04 242 /* Wait for an in-flight buffer to complete and get re-queued */
acd45c56790a3b Takashi Iwai 2022-08-04 243 if (!wait_event_lock_irq_timeout(udl->urbs.sleep,
c5c354a3a47280 Takashi Iwai 2022-09-08 244 !udl->urbs.count ||
acd45c56790a3b Takashi Iwai 2022-08-04 245 !list_empty(&udl->urbs.list),
acd45c56790a3b Takashi Iwai 2022-08-04 246 udl->urbs.lock, timeout)) {
acd45c56790a3b Takashi Iwai 2022-08-04 247 DRM_INFO("wait for urb interrupted: available: %d\n",
acd45c56790a3b Takashi Iwai 2022-08-04 248 udl->urbs.available);
c5c354a3a47280 Takashi Iwai 2022-09-08 249 return NULL;
acd45c56790a3b Takashi Iwai 2022-08-04 250 }
5320918b9a8786 Dave Airlie 2010-12-15 251
c5c354a3a47280 Takashi Iwai 2022-09-08 252 if (!udl->urbs.count)
c5c354a3a47280 Takashi Iwai 2022-09-08 253 return NULL;
c5c354a3a47280 Takashi Iwai 2022-09-08 254
acd45c56790a3b Takashi Iwai 2022-08-04 255 unode = list_first_entry(&udl->urbs.list, struct urb_node, entry);
Use list_first_entry_or_null() if this list can be empty.
ed9605a66b62f2 Takashi Iwai 2022-09-08 256 list_del_init(&unode->entry);
^^^^^^^^^^^^
dereference.
5320918b9a8786 Dave Airlie 2010-12-15 257 udl->urbs.available--;
5320918b9a8786 Dave Airlie 2010-12-15 258
acd45c56790a3b Takashi Iwai 2022-08-04 @259 return unode ? unode->urb : NULL;
Returns from list_first_entry() should never be checked for NULL.
5320918b9a8786 Dave Airlie 2010-12-15 260 }
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
On Tue, 02 May 2023 13:19:04 +0200,
Dan Carpenter wrote:
>
> tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
> head: 2caeeb9d4a1bccd923b7918427f9e9ef7151ddd8
> commit: c5c354a3a4728045e1342166394c615d75d45377 drm/udl: Fix inconsistent urbs.count value during udl_free_urb_list()
> config: parisc-randconfig-m031-20230421 (https://download.01.org/0day-ci/archive/20230423/[email protected]/config)
> compiler: hppa-linux-gcc (GCC) 12.1.0
>
> If you fix the issue, kindly add following tag where applicable
> | Reported-by: kernel test robot <[email protected]>
> | Reported-by: Dan Carpenter <[email protected]>
> | Link: https://lore.kernel.org/r/[email protected]/
>
> smatch warnings:
> drivers/gpu/drm/udl/udl_main.c:259 udl_get_urb_locked() warn: can 'unode' even be NULL?
>
> vim +/unode +259 drivers/gpu/drm/udl/udl_main.c
>
> c5c354a3a47280 Takashi Iwai 2022-09-08 236 static struct urb *udl_get_urb_locked(struct udl_device *udl, long timeout)
> 5320918b9a8786 Dave Airlie 2010-12-15 237 {
> c5c354a3a47280 Takashi Iwai 2022-09-08 238 struct urb_node *unode;
> 5320918b9a8786 Dave Airlie 2010-12-15 239
> c5c354a3a47280 Takashi Iwai 2022-09-08 240 assert_spin_locked(&udl->urbs.lock);
> 5320918b9a8786 Dave Airlie 2010-12-15 241
> acd45c56790a3b Takashi Iwai 2022-08-04 242 /* Wait for an in-flight buffer to complete and get re-queued */
> acd45c56790a3b Takashi Iwai 2022-08-04 243 if (!wait_event_lock_irq_timeout(udl->urbs.sleep,
> c5c354a3a47280 Takashi Iwai 2022-09-08 244 !udl->urbs.count ||
> acd45c56790a3b Takashi Iwai 2022-08-04 245 !list_empty(&udl->urbs.list),
> acd45c56790a3b Takashi Iwai 2022-08-04 246 udl->urbs.lock, timeout)) {
> acd45c56790a3b Takashi Iwai 2022-08-04 247 DRM_INFO("wait for urb interrupted: available: %d\n",
> acd45c56790a3b Takashi Iwai 2022-08-04 248 udl->urbs.available);
> c5c354a3a47280 Takashi Iwai 2022-09-08 249 return NULL;
> acd45c56790a3b Takashi Iwai 2022-08-04 250 }
> 5320918b9a8786 Dave Airlie 2010-12-15 251
> c5c354a3a47280 Takashi Iwai 2022-09-08 252 if (!udl->urbs.count)
> c5c354a3a47280 Takashi Iwai 2022-09-08 253 return NULL;
> c5c354a3a47280 Takashi Iwai 2022-09-08 254
> acd45c56790a3b Takashi Iwai 2022-08-04 255 unode = list_first_entry(&udl->urbs.list, struct urb_node, entry);
>
> Use list_first_entry_or_null() if this list can be empty.
>
> ed9605a66b62f2 Takashi Iwai 2022-09-08 256 list_del_init(&unode->entry);
> ^^^^^^^^^^^^
> dereference.
>
> 5320918b9a8786 Dave Airlie 2010-12-15 257 udl->urbs.available--;
> 5320918b9a8786 Dave Airlie 2010-12-15 258
> acd45c56790a3b Takashi Iwai 2022-08-04 @259 return unode ? unode->urb : NULL;
>
> Returns from list_first_entry() should never be checked for NULL.
>
> 5320918b9a8786 Dave Airlie 2010-12-15 260 }
This is rather a leftover at commit c5c354a3a472 ("drm/udl: Fix
inconsistent urbs.count value during udl_free_urb_list()").
The NULL check of unode at return became superfluous by the code
change there, hence it should be simply like
return ucode->urb;
OTOH, the current code doesn't cause any practical problem.
thanks,
Takashi
Hi
Am 02.05.23 um 13:19 schrieb Dan Carpenter:
> tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
> head: 2caeeb9d4a1bccd923b7918427f9e9ef7151ddd8
> commit: c5c354a3a4728045e1342166394c615d75d45377 drm/udl: Fix inconsistent urbs.count value during udl_free_urb_list()
> config: parisc-randconfig-m031-20230421 (https://download.01.org/0day-ci/archive/20230423/[email protected]/config)
> compiler: hppa-linux-gcc (GCC) 12.1.0
>
> If you fix the issue, kindly add following tag where applicable
> | Reported-by: kernel test robot <[email protected]>
> | Reported-by: Dan Carpenter <[email protected]>
> | Link: https://lore.kernel.org/r/[email protected]/
>
> smatch warnings:
> drivers/gpu/drm/udl/udl_main.c:259 udl_get_urb_locked() warn: can 'unode' even be NULL?
It can't. Send a fix, if you like. OTOH it's not much of a problem either.
Best regards
Thomas
>
> vim +/unode +259 drivers/gpu/drm/udl/udl_main.c
>
> c5c354a3a47280 Takashi Iwai 2022-09-08 236 static struct urb *udl_get_urb_locked(struct udl_device *udl, long timeout)
> 5320918b9a8786 Dave Airlie 2010-12-15 237 {
> c5c354a3a47280 Takashi Iwai 2022-09-08 238 struct urb_node *unode;
> 5320918b9a8786 Dave Airlie 2010-12-15 239
> c5c354a3a47280 Takashi Iwai 2022-09-08 240 assert_spin_locked(&udl->urbs.lock);
> 5320918b9a8786 Dave Airlie 2010-12-15 241
> acd45c56790a3b Takashi Iwai 2022-08-04 242 /* Wait for an in-flight buffer to complete and get re-queued */
> acd45c56790a3b Takashi Iwai 2022-08-04 243 if (!wait_event_lock_irq_timeout(udl->urbs.sleep,
> c5c354a3a47280 Takashi Iwai 2022-09-08 244 !udl->urbs.count ||
> acd45c56790a3b Takashi Iwai 2022-08-04 245 !list_empty(&udl->urbs.list),
> acd45c56790a3b Takashi Iwai 2022-08-04 246 udl->urbs.lock, timeout)) {
> acd45c56790a3b Takashi Iwai 2022-08-04 247 DRM_INFO("wait for urb interrupted: available: %d\n",
> acd45c56790a3b Takashi Iwai 2022-08-04 248 udl->urbs.available);
> c5c354a3a47280 Takashi Iwai 2022-09-08 249 return NULL;
> acd45c56790a3b Takashi Iwai 2022-08-04 250 }
> 5320918b9a8786 Dave Airlie 2010-12-15 251
> c5c354a3a47280 Takashi Iwai 2022-09-08 252 if (!udl->urbs.count)
> c5c354a3a47280 Takashi Iwai 2022-09-08 253 return NULL;
> c5c354a3a47280 Takashi Iwai 2022-09-08 254
> acd45c56790a3b Takashi Iwai 2022-08-04 255 unode = list_first_entry(&udl->urbs.list, struct urb_node, entry);
>
> Use list_first_entry_or_null() if this list can be empty.
>
> ed9605a66b62f2 Takashi Iwai 2022-09-08 256 list_del_init(&unode->entry);
> ^^^^^^^^^^^^
> dereference.
>
> 5320918b9a8786 Dave Airlie 2010-12-15 257 udl->urbs.available--;
> 5320918b9a8786 Dave Airlie 2010-12-15 258
> acd45c56790a3b Takashi Iwai 2022-08-04 @259 return unode ? unode->urb : NULL;
>
> Returns from list_first_entry() should never be checked for NULL.
>
> 5320918b9a8786 Dave Airlie 2010-12-15 260 }
>
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)
On Tue, May 02, 2023 at 01:38:11PM +0200, Thomas Zimmermann wrote:
> Hi
>
> Am 02.05.23 um 13:19 schrieb Dan Carpenter:
> > tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
> > head: 2caeeb9d4a1bccd923b7918427f9e9ef7151ddd8
> > commit: c5c354a3a4728045e1342166394c615d75d45377 drm/udl: Fix inconsistent urbs.count value during udl_free_urb_list()
> > config: parisc-randconfig-m031-20230421 (https://download.01.org/0day-ci/archive/20230423/[email protected]/config)
> > compiler: hppa-linux-gcc (GCC) 12.1.0
> >
> > If you fix the issue, kindly add following tag where applicable
> > | Reported-by: kernel test robot <[email protected]>
> > | Reported-by: Dan Carpenter <[email protected]>
> > | Link: https://lore.kernel.org/r/[email protected]/
> >
> > smatch warnings:
> > drivers/gpu/drm/udl/udl_main.c:259 udl_get_urb_locked() warn: can 'unode' even be NULL?
>
> It can't. Send a fix, if you like. OTOH it's not much of a problem either.
>
Sure, I sent a patch. This one is harmless, but we recently had a few
bugs that should have generated bogus NULL check warnings.
CVE-2023-1073: b12fece4c648 ("HID: check empty report_list in hid_validate_values()")
CVE-2023-1075: ffe2a2256244 ("net/tls: tls_is_tx_ready() checked list_entry")
CVE-2023-1077: 7c4a5b89a0b5 ("sched/rt: pick_next_rt_entity(): check list_entry")
CVE-2023-1078: f753a68980cf ("rds: rds_rm_zerocopy_callback() use list_first_entry()")
Although CVE-2023-1073 and CVE-2023-1077 are hard to warn about without
generating too many false positives.
regards,
dan carpenter