Currently, init_listener() tries to prevent adding a filter with SECCOMP_FILTER_FLAG_NEW_LISTENER if one of the existing filters already has a listener. However, this check happens without holding any lock that would prevent another thread from concurrently installing a new filter (potentially with a listener) on top of the ones we already have. Theoretically, this is also a data race: The plain load from current->seccomp.filter can race with concurrent writes to the same location. Fix it by moving the check into the region that holds the siglock to guard against concurrent TSYNC. (The "Fixes" tag points to the commit that introduced the theoretical data race; concurrent installation of another filter with TSYNC only became possible later, in commit 51891498f2da ("seccomp: allow TSYNC and USER_NOTIF together").)
Unified · Split
| @@ -1476,13 +1476,7 @@ static const struct file_operations seccomp_notify_ops = { · 1475 unmodified lines | |||
| 1476 | 1476 | ||
| 1477 | static struct file *init_listener(struct seccomp_filter *filter) | 1477 | static struct file *init_listener(struct seccomp_filter *filter) |
| 1478 | { | 1478 | { |
| 1479 | struct file *ret = ERR_PTR(-EBUSY); | 1479 | struct file *ret; |
| 1480 | struct seccomp_filter *cur; | ||
| 1481 | |||
| 1482 | for (cur = current->seccomp.filter; cur; cur = cur->prev) { | ||
| 1483 | if (cur->notif) | ||
| 1484 | goto out; | ||
| 1485 | } | ||
| 1486 | 1480 | ||
| 1487 | ret = ERR_PTR(-ENOMEM); | 1481 | ret = ERR_PTR(-ENOMEM); |
| 1488 | filter->notif = kzalloc(sizeof(*(filter->notif)), GFP_KERNEL); | 1482 | filter->notif = kzalloc(sizeof(*(filter->notif)), GFP_KERNEL); |
| @@ -1508,6 +1502,31 @@ out: · 19 unmodified lines | |||
| 1508 | return ret; | 1502 | return ret; |
| 1509 | } | 1503 | } |
| 1510 | 1504 | ||
| 1505 | /* | ||
| 1506 | * Does @new_child have a listener while an ancestor also has a listener? | ||
| 1507 | * If so, we'll want to reject this filter. | ||
| 1508 | * This only has to be tested for the current process, even in the TSYNC case, | ||
| 1509 | * because TSYNC installs @child with the same parent on all threads. | ||
| 1510 | * Note that @new_child is not hooked up to its parent at this point yet, so | ||
| 1511 | * we use current->seccomp.filter. | ||
| 1512 | */ | ||
| 1513 | static bool has_duplicate_listener(struct seccomp_filter *new_child) | ||
| 1514 | { | ||
| 1515 | struct seccomp_filter *cur; | ||
| 1516 | |||
| 1517 | /* must be protected against concurrent TSYNC */ | ||
| 1518 | lockdep_assert_held(¤t->sighand->siglock); | ||
| 1519 | |||
| 1520 | if (!new_child->notif) | ||
| 1521 | return false; | ||
| 1522 | for (cur = current->seccomp.filter; cur; cur = cur->prev) { | ||
| 1523 | if (cur->notif) | ||
| 1524 | return true; | ||
| 1525 | } | ||
| 1526 | |||
| 1527 | return false; | ||
| 1528 | } | ||
| 1529 | |||
| 1511 | /** | 1530 | /** |
| 1512 | * seccomp_set_mode_filter: internal function for setting seccomp filter | 1531 | * seccomp_set_mode_filter: internal function for setting seccomp filter |
| 1513 | * @flags: flags to change filter behavior | 1532 | * @flags: flags to change filter behavior |
| @@ -1579,6 +1598,11 @@ static long seccomp_set_mode_filter(unsigned int flags, · 65 unmodified lines | |||
| 1579 | if (!seccomp_may_assign_mode(seccomp_mode)) | 1598 | if (!seccomp_may_assign_mode(seccomp_mode)) |
| 1580 | goto out; | 1599 | goto out; |
| 1581 | 1600 | ||
| 1601 | if (has_duplicate_listener(prepared)) { | ||
| 1602 | ret = -EBUSY; | ||
| 1603 | goto out; | ||
| 1604 | } | ||
| 1605 | |||
| 1582 | ret = seccomp_attach_filter(flags, prepared); | 1606 | ret = seccomp_attach_filter(flags, prepared); |
| 1583 | if (ret) | 1607 | if (ret) |
| 1584 | goto out; | 1608 | goto out; |