When all files are closed, functionfs needs ffs_data_reset() to be done before any further opens are allowed. During that time we have ffs->state set to FFS_CLOSING; that makes ->open() fail with -EBUSY. Once ffs_data_reset() is done, it switches state (to FFS_READ_DESCRIPTORS) indicating that opening that thing is allowed again. There's a couple of additional twists: * mounting with -o no_disconnect delays ffs_data_reset() from doing that at the final ->release() to the first subsequent open(). That's indicated by ffs->state set to FFS_DEACTIVATED; if open() sees that, it immediately switches to FFS_CLOSING and proceeds with doing ffs_data_reset() before returning to userland. * a couple of usb callbacks need to force the delayed transition; unfortunately, they are done in locking environment that does not allow blocking and ffs_data_reset() can block. As the result, if these callbacks see FFS_DEACTIVATED, they change state to FFS_CLOSING and use schedule_work() to get ffs_data_reset() executed asynchronously. Unfortunately, the locking is rather insufficient. A fix attempted in e5bf5ee26663 ("functionfs: fix the open/removal races") had closed a bunch of UAF, but it didn't do anything to the callbacks, lacked barriers in transition from FFS_CLOSING to FFS_READ_DESCRIPTORS _and_ it had been too heavy-handed in open()/open() serialization - I've used ffs->mutex for that, and it's being held over actual IO on ep0, complete with copy_from_user(), etc. Even more unfortunately, the userland side is apparently racy enough to have the resulting timing changes (no failures, just a delayed return of open(2)) disrupt the things quite badly. Userland bugs or not, it's a clear regression that needs to be dealt with. Solution is to use a spinlock for serializing these state checks and transitions - unlike ffs->mutex it can be taken in these callbacks and it doesn't disrupt the timings in open(). We could introduce a new spinlock, but it's easier to use the one that is already there (ffs->eps_lock) instead - the locking environment is safe for it in all affected places. Since now it is held over all places that alter or check the open count (ffs->opened), there's no need to keep that atomic_t - int would serve just fine and it's simpler that way.
Unified · Split
| @@ -59,7 +59,6 @@ static struct ffs_data *__must_check ffs_data_new(const char *dev_name) · 58 unmodified lines | |||
| 59 | __attribute__((malloc)); | 59 | __attribute__((malloc)); |
| 60 | 60 | ||
| 61 | /* Opened counter handling. */ | 61 | /* Opened counter handling. */ |
| 62 | static void ffs_data_opened(struct ffs_data *ffs); | ||
| 63 | static void ffs_data_closed(struct ffs_data *ffs); | 62 | static void ffs_data_closed(struct ffs_data *ffs); |
| 64 | 63 | ||
| 65 | /* Called with ffs->mutex held; take over ownership of data. */ | 64 | /* Called with ffs->mutex held; take over ownership of data. */ |
| @@ -636,23 +635,25 @@ done_mutex: · 570 unmodified lines | |||
| 636 | return ret; | 635 | return ret; |
| 637 | } | 636 | } |
| 638 | 637 | ||
| 638 | |||
| 639 | static void ffs_data_reset(struct ffs_data *ffs); | ||
| 640 | |||
| 639 | static int ffs_ep0_open(struct inode *inode, struct file *file) | 641 | static int ffs_ep0_open(struct inode *inode, struct file *file) |
| 640 | { | 642 | { |
| 641 | struct ffs_data *ffs = inode->i_sb->s_fs_info; | 643 | struct ffs_data *ffs = inode->i_sb->s_fs_info; |
| 642 | int ret; | ||
| 643 | 644 | ||
| 644 | /* Acquire mutex */ | 645 | spin_lock_irq(&ffs->eps_lock); |
| 645 | ret = ffs_mutex_lock(&ffs->mutex, file->f_flags & O_NONBLOCK); | ||
| 646 | if (ret < 0) | ||
| 647 | return ret; | ||
| 648 | |||
| 649 | ffs_data_opened(ffs); | ||
| 650 | if (ffs->state == FFS_CLOSING) { | 646 | if (ffs->state == FFS_CLOSING) { |
| 651 | ffs_data_closed(ffs); | 647 | spin_unlock_irq(&ffs->eps_lock); |
| 652 | mutex_unlock(&ffs->mutex); | ||
| 653 | return -EBUSY; | 648 | return -EBUSY; |
| 654 | } | 649 | } |
| 655 | mutex_unlock(&ffs->mutex); | 650 | if (!ffs->opened++ && ffs->state == FFS_DEACTIVATED) { |
| 651 | ffs->state = FFS_CLOSING; | ||
| 652 | spin_unlock_irq(&ffs->eps_lock); | ||
| 653 | ffs_data_reset(ffs); | ||
| 654 | } else { | ||
| 655 | spin_unlock_irq(&ffs->eps_lock); | ||
| 656 | } | ||
| 656 | file->private_data = ffs; | 657 | file->private_data = ffs; |
| 657 | 658 | ||
| 658 | return stream_open(inode, file); | 659 | return stream_open(inode, file); |
| @@ -1202,15 +1203,10 @@ ffs_epfile_open(struct inode *inode, struct file *file) · 543 unmodified lines | |||
| 1202 | { | 1203 | { |
| 1203 | struct ffs_data *ffs = inode->i_sb->s_fs_info; | 1204 | struct ffs_data *ffs = inode->i_sb->s_fs_info; |
| 1204 | struct ffs_epfile *epfile; | 1205 | struct ffs_epfile *epfile; |
| 1205 | int ret; | ||
| 1206 | |||
| 1207 | /* Acquire mutex */ | ||
| 1208 | ret = ffs_mutex_lock(&ffs->mutex, file->f_flags & O_NONBLOCK); | ||
| 1209 | if (ret < 0) | ||
| 1210 | return ret; | ||
| 1211 | 1206 | ||
| 1212 | if (!atomic_inc_not_zero(&ffs->opened)) { | 1207 | spin_lock_irq(&ffs->eps_lock); |
| 1213 | mutex_unlock(&ffs->mutex); | 1208 | if (!ffs->opened) { |
| 1209 | spin_unlock_irq(&ffs->eps_lock); | ||
| 1214 | return -ENODEV; | 1210 | return -ENODEV; |
| 1215 | } | 1211 | } |
| 1216 | /* | 1212 | /* |
| @@ -1220,11 +1216,11 @@ ffs_epfile_open(struct inode *inode, struct file *file) · 3 unmodified lines | |||
| 1220 | */ | 1216 | */ |
| 1221 | epfile = smp_load_acquire(&inode->i_private); | 1217 | epfile = smp_load_acquire(&inode->i_private); |
| 1222 | if (unlikely(ffs->state != FFS_ACTIVE || !epfile)) { | 1218 | if (unlikely(ffs->state != FFS_ACTIVE || !epfile)) { |
| 1223 | mutex_unlock(&ffs->mutex); | 1219 | spin_unlock_irq(&ffs->eps_lock); |
| 1224 | ffs_data_closed(ffs); | ||
| 1225 | return -ENODEV; | 1220 | return -ENODEV; |
| 1226 | } | 1221 | } |
| 1227 | mutex_unlock(&ffs->mutex); | 1222 | ffs->opened++; |
| 1223 | spin_unlock_irq(&ffs->eps_lock); | ||
| 1228 | 1224 | ||
| 1229 | file->private_data = epfile; | 1225 | file->private_data = epfile; |
| 1230 | return stream_open(inode, file); | 1226 | return stream_open(inode, file); |
| @@ -2092,8 +2088,6 @@ static int ffs_fs_init_fs_context(struct fs_context *fc) · 861 unmodified lines | |||
| 2092 | return 0; | 2088 | return 0; |
| 2093 | } | 2089 | } |
| 2094 | 2090 | ||
| 2095 | static void ffs_data_reset(struct ffs_data *ffs); | ||
| 2096 | |||
| 2097 | static void | 2091 | static void |
| 2098 | ffs_fs_kill_sb(struct super_block *sb) | 2092 | ffs_fs_kill_sb(struct super_block *sb) |
| 2099 | { | 2093 | { |
| @@ -2150,15 +2144,6 @@ static void ffs_data_get(struct ffs_data *ffs) · 50 unmodified lines | |||
| 2150 | refcount_inc(&ffs->ref); | 2144 | refcount_inc(&ffs->ref); |
| 2151 | } | 2145 | } |
| 2152 | 2146 | ||
| 2153 | static void ffs_data_opened(struct ffs_data *ffs) | ||
| 2154 | { | ||
| 2155 | if (atomic_add_return(1, &ffs->opened) == 1 && | ||
| 2156 | ffs->state == FFS_DEACTIVATED) { | ||
| 2157 | ffs->state = FFS_CLOSING; | ||
| 2158 | ffs_data_reset(ffs); | ||
| 2159 | } | ||
| 2160 | } | ||
| 2161 | |||
| 2162 | static void ffs_data_put(struct ffs_data *ffs) | 2147 | static void ffs_data_put(struct ffs_data *ffs) |
| 2163 | { | 2148 | { |
| 2164 | if (refcount_dec_and_test(&ffs->ref)) { | 2149 | if (refcount_dec_and_test(&ffs->ref)) { |
| @@ -2176,28 +2161,29 @@ static void ffs_data_put(struct ffs_data *ffs) · 11 unmodified lines | |||
| 2176 | 2161 | ||
| 2177 | static void ffs_data_closed(struct ffs_data *ffs) | 2162 | static void ffs_data_closed(struct ffs_data *ffs) |
| 2178 | { | 2163 | { |
| 2179 | if (atomic_dec_and_test(&ffs->opened)) { | 2164 | spin_lock_irq(&ffs->eps_lock); |
| 2180 | if (ffs->no_disconnect) { | 2165 | if (--ffs->opened) { // not the last opener? |
| 2181 | struct ffs_epfile *epfiles; | 2166 | spin_unlock_irq(&ffs->eps_lock); |
| 2182 | unsigned long flags; | 2167 | return; |
| 2183 | 2168 | } | |
| 2184 | ffs->state = FFS_DEACTIVATED; | 2169 | if (ffs->no_disconnect) { |
| 2185 | spin_lock_irqsave(&ffs->eps_lock, flags); | 2170 | struct ffs_epfile *epfiles; |
| 2186 | epfiles = ffs->epfiles; | 2171 | |
| 2187 | ffs->epfiles = NULL; | 2172 | ffs->state = FFS_DEACTIVATED; |
| 2188 | spin_unlock_irqrestore(&ffs->eps_lock, | 2173 | epfiles = ffs->epfiles; |
| 2189 | flags); | 2174 | ffs->epfiles = NULL; |
| 2190 | 2175 | spin_unlock_irq(&ffs->eps_lock); | |
| 2191 | if (epfiles) | 2176 | |
| 2192 | ffs_epfiles_destroy(ffs->sb, epfiles, | 2177 | if (epfiles) |
| 2193 | ffs->eps_count); | 2178 | ffs_epfiles_destroy(ffs->sb, epfiles, |
| 2194 | 2179 | ffs->eps_count); | |
| 2195 | if (ffs->setup_state == FFS_SETUP_PENDING) | 2180 | |
| 2196 | __ffs_ep0_stall(ffs); | 2181 | if (ffs->setup_state == FFS_SETUP_PENDING) |
| 2197 | } else { | 2182 | __ffs_ep0_stall(ffs); |
| 2198 | ffs->state = FFS_CLOSING; | 2183 | } else { |
| 2199 | ffs_data_reset(ffs); | 2184 | ffs->state = FFS_CLOSING; |
| 2200 | } | 2185 | spin_unlock_irq(&ffs->eps_lock); |
| 2186 | ffs_data_reset(ffs); | ||
| 2201 | } | 2187 | } |
| 2202 | } | 2188 | } |
| 2203 | 2189 | ||
| @@ -2214,7 +2200,7 @@ static struct ffs_data *ffs_data_new(const char *dev_name) · 10 unmodified lines | |||
| 2214 | } | 2200 | } |
| 2215 | 2201 | ||
| 2216 | refcount_set(&ffs->ref, 1); | 2202 | refcount_set(&ffs->ref, 1); |
| 2217 | atomic_set(&ffs->opened, 0); | 2203 | ffs->opened = 0; |
| 2218 | ffs->state = FFS_READ_DESCRIPTORS; | 2204 | ffs->state = FFS_READ_DESCRIPTORS; |
| 2219 | mutex_init(&ffs->mutex); | 2205 | mutex_init(&ffs->mutex); |
| 2220 | spin_lock_init(&ffs->eps_lock); | 2206 | spin_lock_init(&ffs->eps_lock); |
| @@ -2266,6 +2252,7 @@ static void ffs_data_reset(struct ffs_data *ffs) · 45 unmodified lines | |||
| 2266 | { | 2252 | { |
| 2267 | ffs_data_clear(ffs); | 2253 | ffs_data_clear(ffs); |
| 2268 | 2254 | ||
| 2255 | spin_lock_irq(&ffs->eps_lock); | ||
| 2269 | ffs->raw_descs_data = NULL; | 2256 | ffs->raw_descs_data = NULL; |
| 2270 | ffs->raw_descs = NULL; | 2257 | ffs->raw_descs = NULL; |
| 2271 | ffs->raw_strings = NULL; | 2258 | ffs->raw_strings = NULL; |
| @@ -2289,6 +2276,7 @@ static void ffs_data_reset(struct ffs_data *ffs) · 17 unmodified lines | |||
| 2289 | ffs->ms_os_descs_ext_prop_count = 0; | 2276 | ffs->ms_os_descs_ext_prop_count = 0; |
| 2290 | ffs->ms_os_descs_ext_prop_name_len = 0; | 2277 | ffs->ms_os_descs_ext_prop_name_len = 0; |
| 2291 | ffs->ms_os_descs_ext_prop_data_len = 0; | 2278 | ffs->ms_os_descs_ext_prop_data_len = 0; |
| 2279 | spin_unlock_irq(&ffs->eps_lock); | ||
| 2292 | } | 2280 | } |
| 2293 | 2281 | ||
| 2294 | 2282 | ||
| @@ -3756,6 +3744,7 @@ static int ffs_func_set_alt(struct usb_function *f, · 1461 unmodified lines | |||
| 3756 | { | 3744 | { |
| 3757 | struct ffs_function *func = ffs_func_from_usb(f); | 3745 | struct ffs_function *func = ffs_func_from_usb(f); |
| 3758 | struct ffs_data *ffs = func->ffs; | 3746 | struct ffs_data *ffs = func->ffs; |
| 3747 | unsigned long flags; | ||
| 3759 | int ret = 0, intf; | 3748 | int ret = 0, intf; |
| 3760 | 3749 | ||
| 3761 | if (alt > MAX_ALT_SETTINGS) | 3750 | if (alt > MAX_ALT_SETTINGS) |
| @@ -3768,12 +3757,15 @@ static int ffs_func_set_alt(struct usb_function *f, · 6 unmodified lines | |||
| 3768 | if (ffs->func) | 3757 | if (ffs->func) |
| 3769 | ffs_func_eps_disable(ffs->func); | 3758 | ffs_func_eps_disable(ffs->func); |
| 3770 | 3759 | ||
| 3760 | spin_lock_irqsave(&ffs->eps_lock, flags); | ||
| 3771 | if (ffs->state == FFS_DEACTIVATED) { | 3761 | if (ffs->state == FFS_DEACTIVATED) { |
| 3772 | ffs->state = FFS_CLOSING; | 3762 | ffs->state = FFS_CLOSING; |
| 3763 | spin_unlock_irqrestore(&ffs->eps_lock, flags); | ||
| 3773 | INIT_WORK(&ffs->reset_work, ffs_reset_work); | 3764 | INIT_WORK(&ffs->reset_work, ffs_reset_work); |
| 3774 | schedule_work(&ffs->reset_work); | 3765 | schedule_work(&ffs->reset_work); |
| 3775 | return -ENODEV; | 3766 | return -ENODEV; |
| 3776 | } | 3767 | } |
| 3768 | spin_unlock_irqrestore(&ffs->eps_lock, flags); | ||
| 3777 | 3769 | ||
| 3778 | if (ffs->state != FFS_ACTIVE) | 3770 | if (ffs->state != FFS_ACTIVE) |
| 3779 | return -ENODEV; | 3771 | return -ENODEV; |
| @@ -3791,16 +3783,20 @@ static void ffs_func_disable(struct usb_function *f) · 11 unmodified lines | |||
| 3791 | { | 3783 | { |
| 3792 | struct ffs_function *func = ffs_func_from_usb(f); | 3784 | struct ffs_function *func = ffs_func_from_usb(f); |
| 3793 | struct ffs_data *ffs = func->ffs; | 3785 | struct ffs_data *ffs = func->ffs; |
| 3786 | unsigned long flags; | ||
| 3794 | 3787 | ||
| 3795 | if (ffs->func) | 3788 | if (ffs->func) |
| 3796 | ffs_func_eps_disable(ffs->func); | 3789 | ffs_func_eps_disable(ffs->func); |
| 3797 | 3790 | ||
| 3791 | spin_lock_irqsave(&ffs->eps_lock, flags); | ||
| 3798 | if (ffs->state == FFS_DEACTIVATED) { | 3792 | if (ffs->state == FFS_DEACTIVATED) { |
| 3799 | ffs->state = FFS_CLOSING; | 3793 | ffs->state = FFS_CLOSING; |
| 3794 | spin_unlock_irqrestore(&ffs->eps_lock, flags); | ||
| 3800 | INIT_WORK(&ffs->reset_work, ffs_reset_work); | 3795 | INIT_WORK(&ffs->reset_work, ffs_reset_work); |
| 3801 | schedule_work(&ffs->reset_work); | 3796 | schedule_work(&ffs->reset_work); |
| 3802 | return; | 3797 | return; |
| 3803 | } | 3798 | } |
| 3799 | spin_unlock_irqrestore(&ffs->eps_lock, flags); | ||
| 3804 | 3800 | ||
| 3805 | if (ffs->state == FFS_ACTIVE) { | 3801 | if (ffs->state == FFS_ACTIVE) { |
| 3806 | ffs->func = NULL; | 3802 | ffs->func = NULL; |
| @@ -176,7 +176,7 @@ struct ffs_data { · 175 unmodified lines | |||
| 176 | /* reference counter */ | 176 | /* reference counter */ |
| 177 | refcount_t ref; | 177 | refcount_t ref; |
| 178 | /* how many files are opened (EP0 and others) */ | 178 | /* how many files are opened (EP0 and others) */ |
| 179 | atomic_t opened; | 179 | int opened; |
| 180 | 180 | ||
| 181 | /* EP0 state */ | 181 | /* EP0 state */ |
| 182 | enum ffs_state state; | 182 | enum ffs_state state; |