netdev - Re: [PATCH net v4] nfc: ... device_is_registered() is data race-able

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 28 Apr 2022 15:15:02 +0800 (GMT+08:00)
From:   "Lin Ma" <linma@....edu.cn>
To:     "Jakub Kicinski" <kuba@...nel.org>
Cc:     "Duoming Zhou" <duoming@....edu.cn>,
        krzysztof.kozlowski@...aro.org, pabeni@...hat.com,
        linux-kernel@...r.kernel.org, davem@...emloft.net,
        gregkh@...uxfoundation.org, alexander.deucher@....com,
        akpm@...ux-foundation.org, broonie@...nel.org,
        netdev@...r.kernel.org
Subject: Re: [PATCH net v4] nfc: ... device_is_registered() is data
 race-able

Hello Jakub,

and hello there maintainers, when we tried to fix this race problem, we found another very weird issue as below

> 
> You can't use a single global variable, there can be many devices 
> each with their own lock.
> 
> Paolo suggested adding a lock, if spin lock doesn't fit the bill
> why not add a mutex?

The lock patch can be added to nfcmrvl code but we prefer to fix this in the NFC core layer hence every other driver that supports the firmware downloading task will be free of such a problem.

But when we analyze the race between the netlink tasks and the cleanup routine, we find some *condition checks* fail to fulfill their responsibility.

For example, we once though that the device_lock + device_is_registered check can help to fix the race as below.

  netlink task              |  cleanup routine
                            |
nfc_genl_fw_download        | nfc_unregister_device 
  nfc_fw_download           |   device_del 
    device_lock             |     device_lock
    // wait lock            |       kobject_del
    // ...                  |         ...
    device_is_registered    |     device_unlock    
      rc = -ENODEV          |

However, by dynamic debugging this issue, we find out that **even after the device_del**, the device_is_registered check still returns TRUE!

This is by no means matching our expectations as one of our previous patch relies on the device_is_registered code.

-> the patch: 3e3b5dfcd16a ("NFC: reorder the logic in nfc_{un,}register_device")

To find out why, we find out the device_is_registered is implemented like below:

static inline int device_is_registered(struct device *dev)
{
	return dev->kobj.state_in_sysfs;
}

By debugging, we find out in normal case, this kobj.state_in_sysfs will be clear out like below

[#0] 0xffffffff81f0743a → __kobject_del(kobj=0xffff888009ca7018)
[#1] 0xffffffff81f07882 → kobject_del(kobj=0xffff888009ca7018)
[#2] 0xffffffff81f07882 → kobject_del(kobj=0xffff888009ca7018)
[#3] 0xffffffff827708db → device_del(dev=0xffff888009ca7018)
[#4] 0xffffffff8396496f → nfc_unregister_device(dev=0xffff888009ca7000)
[#5] 0xffffffff839850a9 → nci_unregister_device(ndev=0xffff888009ca3000)
[#6] 0xffffffff82811308 → nfcmrvl_nci_unregister_dev(priv=0xffff88800c805c00)
[#7] 0xffffffff83990c4f → nci_uart_tty_close(tty=0xffff88800b450000)
[#8] 0xffffffff820f6bd3 → tty_ldisc_kill(tty=0xffff88800b450000)
[#9] 0xffffffff820f7fb1 → tty_ldisc_hangup(tty=0xffff88800b450000, reinit=0x0)

The clear out is in function __kobject_del

static void __kobject_del(struct kobject *kobj)
{
   // ...

	kobj->state_in_sysfs = 0;
	kobj_kset_leave(kobj);
	kobj->parent = NULL;
}

The structure of device_del is like below

void device_del(struct device *dev)
{
	struct device *parent = dev->parent;
	struct kobject *glue_dir = NULL;
	struct class_interface *class_intf;
	unsigned int noio_flag;

	device_lock(dev);
	kill_device(dev);
	device_unlock(dev);

        // ...
        kobject_del(&dev->kobj);
	cleanup_glue_dir(dev, glue_dir);
	memalloc_noio_restore(noio_flag);
	put_device(parent);
}

In another word, the device_del -> kobject_del -> __kobject_del is not protected by the device_lock.

This means the device_lock + device_is_registered is still prone to the data race. And this is not just the problem with firmware downloading. The all relevant netlink tasks that use the device_lock + device_is_registered is possible to be raced.

To this end, we will come out with two patches, one for fixing this device_is_registered by using another status variable instead. The other is the patch that reorders the code in nci_unregister_device.

Thanks
Lin Ma