The following changes since commit 609488bc979f99f805f34e9a32c1e3b71179d10b:
Linux 5.3-rc2 (2019-07-28 12:47:02 -0700)
are available in the Git repository at:
https://github.com/micah-morton/linux.git tags/safesetid-bugfix-5.4
for you to fetch changes up to 21ab8580b383f27b7f59b84ac1699cb26d6c3d69:
LSM: SafeSetID: Stop releasing uninitialized ruleset (2019-09-17
11:27:05 -0700)
----------------------------------------------------------------
Fix for SafeSetID bug that was introduced in 5.3
Jann Horn sent some patches to fix some bugs in SafeSetID for 5.3. After
he had done his testing there were a couple small code tweaks that went
in and caused this bug. From what I can see SafeSetID is broken in 5.3
and crashes the kernel every time during initialization if you try to
use it. I came across this bug when backporting Jann's changes for 5.3
to older kernels (4.14 and 4.19). I've tested on a Chrome OS device and
verified that this change fixes things. Unless I'm missing something it
doesn't seem super useful to have this change bake in linux-next, since it is
completely broken in 5.3 and nobody noticed.
Signed-off-by: Micah Morton <[email protected]>
----------------------------------------------------------------
Micah Morton (1):
LSM: SafeSetID: Stop releasing uninitialized ruleset
security/safesetid/securityfs.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
On Mon, Sep 23, 2019 at 12:01 PM Linus Torvalds
<[email protected]> wrote:
>
> Anyway, this bug would likely had been avoided if rcu_swap_protected()
> just returned the old pointer instead of changing the argument.
Also, I have to say that the fact that I got the fundamentally buggy
commit in a pull request during the 5.3 merge window, and merged it
on July 16, but then get the pull request for the fix two months
later, after 5.3 has been released, makes me very unhappy with the
state of safesetid.
The pull request itself was clearly never tested. That's a big problem.
And *nobody* used it at all or tested it at all during the whole
release process. That's another big problem.
Should we just remove safesetid again? It's not really maintained, and
it's apparently not used. It was merged in March (with the first
commit in January), and here we are at end of September and this
happens.
So yes, syntactically I'll blame the bad RCU interfaces for why the
bug happened.
But the fact that the code didn't _work_ and was never tested by
anybody for two months, that's not the fault of the RCU code.
Linus
On Mon, Sep 23, 2019 at 12:01:49PM -0700, Linus Torvalds wrote:
> On Wed, Sep 18, 2019 at 10:41 AM Micah Morton <[email protected]> wrote:
> >
> > Fix for SafeSetID bug that was introduced in 5.3
>
> So this seems to be a good fix, but the bug itself came from the fact that
>
> rcu_swap_protected(..)
>
> is so hard to read, and I don't see *why* it's so pointlessly hard to read.
>
> Yes, we have some macros that change their arguments, but they have a
> _reason_ to do so (ie they return two different values) and they tend
> to be very special in other ways too.
>
> But rcu_swap_protected() has no reason for it's odd semantics.
>
> Looking at that 'handle_policy_update()' function, it's entirely
> reasonable to think "pol cannot possibly be NULL". When I looked at
> the fix patch, that was my initial reaction too, and it's probably the
> reason Jann's commit 03638e62f55f ("LSM: SafeSetID: rewrite userspace
> API to atomic updates") had that bug to begin with.
>
> I don't see the original discussion at all, it's not on
> Linux-Security-Module for some reason, so I can't tell when/if the
> NULL pointer test got deleted.
>
> Anyway, this bug would likely had been avoided if rcu_swap_protected()
> just returned the old pointer instead of changing the argument.
>
> There are only a handful or users of that macro, maybe this could be fixed?
I pushed some (untested) commits out to the dev branch of -rcu, the
overall effect of which is shown in the patch below. The series
adds a new rcu_replace() to avoid confusion with swap(), replaces
uses of rcu_swap_protected() with rcu_replace(), and finally removes
rcu_swap_protected().
Is this what you had in mind?
Unless you tell me otherwise, I will assume that this change is important
but not violently urgent. (As in not for the current merge window.)
Thanx, Paul
------------------------------------------------------------------------
diff --git a/arch/x86/kvm/pmu.c b/arch/x86/kvm/pmu.c
index 46875bb..4c37266 100644
--- a/arch/x86/kvm/pmu.c
+++ b/arch/x86/kvm/pmu.c
@@ -416,8 +416,8 @@ int kvm_vm_ioctl_set_pmu_event_filter(struct kvm *kvm, void __user *argp)
*filter = tmp;
mutex_lock(&kvm->lock);
- rcu_swap_protected(kvm->arch.pmu_event_filter, filter,
- mutex_is_locked(&kvm->lock));
+ filter = rcu_replace(kvm->arch.pmu_event_filter, filter,
+ mutex_is_locked(&kvm->lock));
mutex_unlock(&kvm->lock);
synchronize_srcu_expedited(&kvm->srcu);
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c
index 0f2c22a..ebb4f15 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
@@ -1683,7 +1683,7 @@ set_engines(struct i915_gem_context *ctx,
i915_gem_context_set_user_engines(ctx);
else
i915_gem_context_clear_user_engines(ctx);
- rcu_swap_protected(ctx->engines, set.engines, 1);
+ set.engines = rcu_replace(ctx->engines, set.engines, 1);
mutex_unlock(&ctx->engines_mutex);
call_rcu(&set.engines->rcu, free_engines_rcu);
diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index 1f5b5c8..6a38d4a 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -434,8 +434,8 @@ static void scsi_update_vpd_page(struct scsi_device *sdev, u8 page,
return;
mutex_lock(&sdev->inquiry_mutex);
- rcu_swap_protected(*sdev_vpd_buf, vpd_buf,
- lockdep_is_held(&sdev->inquiry_mutex));
+ vpd_buf = rcu_replace(*sdev_vpd_buf, vpd_buf,
+ lockdep_is_held(&sdev->inquiry_mutex));
mutex_unlock(&sdev->inquiry_mutex);
if (vpd_buf)
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index 64c96c7..8d17779 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -466,10 +466,10 @@ static void scsi_device_dev_release_usercontext(struct work_struct *work)
sdev->request_queue = NULL;
mutex_lock(&sdev->inquiry_mutex);
- rcu_swap_protected(sdev->vpd_pg80, vpd_pg80,
- lockdep_is_held(&sdev->inquiry_mutex));
- rcu_swap_protected(sdev->vpd_pg83, vpd_pg83,
- lockdep_is_held(&sdev->inquiry_mutex));
+ vpd_pg80 = rcu_replace(sdev->vpd_pg80, vpd_pg80,
+ lockdep_is_held(&sdev->inquiry_mutex));
+ vpd_pg83 = rcu_replace(sdev->vpd_pg83, vpd_pg83,
+ lockdep_is_held(&sdev->inquiry_mutex));
mutex_unlock(&sdev->inquiry_mutex);
if (vpd_pg83)
diff --git a/fs/afs/vl_list.c b/fs/afs/vl_list.c
index 21eb0c0..e594598 100644
--- a/fs/afs/vl_list.c
+++ b/fs/afs/vl_list.c
@@ -279,8 +279,8 @@ struct afs_vlserver_list *afs_extract_vlserver_list(struct afs_cell *cell,
struct afs_addr_list *old = addrs;
write_lock(&server->lock);
- rcu_swap_protected(server->addresses, old,
- lockdep_is_held(&server->lock));
+ old = rcu_replace(server->addresses, old,
+ lockdep_is_held(&server->lock));
write_unlock(&server->lock);
afs_put_addrlist(old);
}
diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
index 80d6056..968d258 100644
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -383,20 +383,22 @@ do { \
} while (0)
/**
- * rcu_swap_protected() - swap an RCU and a regular pointer
- * @rcu_ptr: RCU pointer
+ * rcu_replace() - replace an RCU pointer, returning its old value
+ * @rcu_ptr: RCU pointer, whose old value is returned
* @ptr: regular pointer
- * @c: the conditions under which the dereference will take place
+ * @c: the lockdep conditions under which the dereference will take place
*
- * Perform swap(@rcu_ptr, @ptr) where @rcu_ptr is an RCU-annotated pointer and
- * @c is the argument that is passed to the rcu_dereference_protected() call
- * used to read that pointer.
+ * Perform a replacement, where @rcu_ptr is an RCU-annotated
+ * pointer and @c is the lockdep argument that is passed to the
+ * rcu_dereference_protected() call used to read that pointer. The old
+ * value of @rcu_ptr is returned, and @rcu_ptr is set to @ptr.
*/
-#define rcu_swap_protected(rcu_ptr, ptr, c) do { \
+#define rcu_replace(rcu_ptr, ptr, c) \
+({ \
typeof(ptr) __tmp = rcu_dereference_protected((rcu_ptr), (c)); \
rcu_assign_pointer((rcu_ptr), (ptr)); \
- (ptr) = __tmp; \
-} while (0)
+ __tmp; \
+})
/**
* rcu_access_pointer() - fetch RCU pointer with no dereferencing
diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
index 0a00eac..cee7f08 100644
--- a/kernel/bpf/cgroup.c
+++ b/kernel/bpf/cgroup.c
@@ -180,8 +180,8 @@ static void activate_effective_progs(struct cgroup *cgrp,
enum bpf_attach_type type,
struct bpf_prog_array *old_array)
{
- rcu_swap_protected(cgrp->bpf.effective[type], old_array,
- lockdep_is_held(&cgroup_mutex));
+ old_array = rcu_replace(cgrp->bpf.effective[type], old_array,
+ lockdep_is_held(&cgroup_mutex));
/* free prog array after grace period, since __cgroup_bpf_run_*()
* might be still walking the array
*/
diff --git a/net/core/dev.c b/net/core/dev.c
index fc676b2..c60454d 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1288,8 +1288,8 @@ int dev_set_alias(struct net_device *dev, const char *alias, size_t len)
}
mutex_lock(&ifalias_mutex);
- rcu_swap_protected(dev->ifalias, new_alias,
- mutex_is_locked(&ifalias_mutex));
+ new_alias = rcu_replace(dev->ifalias, new_alias,
+ mutex_is_locked(&ifalias_mutex));
mutex_unlock(&ifalias_mutex);
if (new_alias)
diff --git a/net/core/sock_reuseport.c b/net/core/sock_reuseport.c
index 9408f92..635d202 100644
--- a/net/core/sock_reuseport.c
+++ b/net/core/sock_reuseport.c
@@ -345,8 +345,8 @@ int reuseport_detach_prog(struct sock *sk)
spin_lock_bh(&reuseport_lock);
reuse = rcu_dereference_protected(sk->sk_reuseport_cb,
lockdep_is_held(&reuseport_lock));
- rcu_swap_protected(reuse->prog, old_prog,
- lockdep_is_held(&reuseport_lock));
+ old_prog = rcu_replace(reuse->prog, old_prog,
+ lockdep_is_held(&reuseport_lock));
spin_unlock_bh(&reuseport_lock);
if (!old_prog)
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 605a7cf..ea8f7cf 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -1456,8 +1456,9 @@ static void nft_chain_stats_replace(struct nft_trans *trans)
if (!nft_trans_chain_stats(trans))
return;
- rcu_swap_protected(chain->stats, nft_trans_chain_stats(trans),
- lockdep_commit_lock_is_held(trans->ctx.net));
+ nft_trans_chain_stats(trans) =
+ rcu_replace(chain->stats, nft_trans_chain_stats(trans),
+ lockdep_commit_lock_is_held(trans->ctx.net));
if (!nft_trans_chain_stats(trans))
static_branch_inc(&nft_counters_enabled);
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index 3397122..56fe6f8 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -88,7 +88,7 @@ struct tcf_chain *tcf_action_set_ctrlact(struct tc_action *a, int action,
struct tcf_chain *goto_chain)
{
a->tcfa_action = action;
- rcu_swap_protected(a->goto_chain, goto_chain, 1);
+ goto_chain = rcu_replace(a->goto_chain, goto_chain, 1);
return goto_chain;
}
EXPORT_SYMBOL(tcf_action_set_ctrlact);
diff --git a/net/sched/act_csum.c b/net/sched/act_csum.c
index 621fb22..d4c5713 100644
--- a/net/sched/act_csum.c
+++ b/net/sched/act_csum.c
@@ -100,8 +100,8 @@ static int tcf_csum_init(struct net *net, struct nlattr *nla,
spin_lock_bh(&p->tcf_lock);
goto_ch = tcf_action_set_ctrlact(*a, parm->action, goto_ch);
- rcu_swap_protected(p->params, params_new,
- lockdep_is_held(&p->tcf_lock));
+ params_new = rcu_replace(p->params, params_new,
+ lockdep_is_held(&p->tcf_lock));
spin_unlock_bh(&p->tcf_lock);
if (goto_ch)
diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c
index b501ce0..167baaf 100644
--- a/net/sched/act_ct.c
+++ b/net/sched/act_ct.c
@@ -721,7 +721,7 @@ static int tcf_ct_init(struct net *net, struct nlattr *nla,
spin_lock_bh(&c->tcf_lock);
goto_ch = tcf_action_set_ctrlact(*a, parm->action, goto_ch);
- rcu_swap_protected(c->params, params, lockdep_is_held(&c->tcf_lock));
+ params = rcu_replace(c->params, params, lockdep_is_held(&c->tcf_lock));
spin_unlock_bh(&c->tcf_lock);
if (goto_ch)
diff --git a/net/sched/act_ctinfo.c b/net/sched/act_ctinfo.c
index 10eb2bb..6d5a34d 100644
--- a/net/sched/act_ctinfo.c
+++ b/net/sched/act_ctinfo.c
@@ -256,8 +256,8 @@ static int tcf_ctinfo_init(struct net *net, struct nlattr *nla,
spin_lock_bh(&ci->tcf_lock);
goto_ch = tcf_action_set_ctrlact(*a, actparm->action, goto_ch);
- rcu_swap_protected(ci->params, cp_new,
- lockdep_is_held(&ci->tcf_lock));
+ cp_new = rcu_replace(ci->params, cp_new,
+ lockdep_is_held(&ci->tcf_lock));
spin_unlock_bh(&ci->tcf_lock);
if (goto_ch)
diff --git a/net/sched/act_ife.c b/net/sched/act_ife.c
index 41d5398..eea4772 100644
--- a/net/sched/act_ife.c
+++ b/net/sched/act_ife.c
@@ -587,7 +587,7 @@ static int tcf_ife_init(struct net *net, struct nlattr *nla,
spin_lock_bh(&ife->tcf_lock);
/* protected by tcf_lock when modifying existing action */
goto_ch = tcf_action_set_ctrlact(*a, parm->action, goto_ch);
- rcu_swap_protected(ife->params, p, 1);
+ p = rcu_replace(ife->params, p, 1);
if (exists)
spin_unlock_bh(&ife->tcf_lock);
diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
index 055faa2..72ec97c 100644
--- a/net/sched/act_mirred.c
+++ b/net/sched/act_mirred.c
@@ -177,8 +177,8 @@ static int tcf_mirred_init(struct net *net, struct nlattr *nla,
goto put_chain;
}
mac_header_xmit = dev_is_mac_header_xmit(dev);
- rcu_swap_protected(m->tcfm_dev, dev,
- lockdep_is_held(&m->tcf_lock));
+ dev = rcu_replace(m->tcfm_dev, dev,
+ lockdep_is_held(&m->tcf_lock));
if (dev)
dev_put(dev);
m->tcfm_mac_header_xmit = mac_header_xmit;
diff --git a/net/sched/act_mpls.c b/net/sched/act_mpls.c
index ca2597c..e33ce9c 100644
--- a/net/sched/act_mpls.c
+++ b/net/sched/act_mpls.c
@@ -256,7 +256,7 @@ static int tcf_mpls_init(struct net *net, struct nlattr *nla,
spin_lock_bh(&m->tcf_lock);
goto_ch = tcf_action_set_ctrlact(*a, parm->action, goto_ch);
- rcu_swap_protected(m->mpls_p, p, lockdep_is_held(&m->tcf_lock));
+ p = rcu_replace(m->mpls_p, p, lockdep_is_held(&m->tcf_lock));
spin_unlock_bh(&m->tcf_lock);
if (goto_ch)
diff --git a/net/sched/act_police.c b/net/sched/act_police.c
index a065f62..6d4817e 100644
--- a/net/sched/act_police.c
+++ b/net/sched/act_police.c
@@ -182,9 +182,9 @@ static int tcf_police_init(struct net *net, struct nlattr *nla,
police->tcfp_ptoks = new->tcfp_mtu_ptoks;
spin_unlock_bh(&police->tcfp_lock);
goto_ch = tcf_action_set_ctrlact(*a, parm->action, goto_ch);
- rcu_swap_protected(police->params,
- new,
- lockdep_is_held(&police->tcf_lock));
+ new = rcu_replace(police->params,
+ new,
+ lockdep_is_held(&police->tcf_lock));
spin_unlock_bh(&police->tcf_lock);
if (goto_ch)
diff --git a/net/sched/act_skbedit.c b/net/sched/act_skbedit.c
index 215a067..8ca1b8a 100644
--- a/net/sched/act_skbedit.c
+++ b/net/sched/act_skbedit.c
@@ -205,8 +205,8 @@ static int tcf_skbedit_init(struct net *net, struct nlattr *nla,
spin_lock_bh(&d->tcf_lock);
goto_ch = tcf_action_set_ctrlact(*a, parm->action, goto_ch);
- rcu_swap_protected(d->params, params_new,
- lockdep_is_held(&d->tcf_lock));
+ params_new = rcu_replace(d->params, params_new,
+ lockdep_is_held(&d->tcf_lock));
spin_unlock_bh(&d->tcf_lock);
if (params_new)
kfree_rcu(params_new, rcu);
diff --git a/net/sched/act_tunnel_key.c b/net/sched/act_tunnel_key.c
index 10dffda..a1f6ede 100644
--- a/net/sched/act_tunnel_key.c
+++ b/net/sched/act_tunnel_key.c
@@ -379,8 +379,8 @@ static int tunnel_key_init(struct net *net, struct nlattr *nla,
spin_lock_bh(&t->tcf_lock);
goto_ch = tcf_action_set_ctrlact(*a, parm->action, goto_ch);
- rcu_swap_protected(t->params, params_new,
- lockdep_is_held(&t->tcf_lock));
+ params_new = rcu_replace(t->params, params_new,
+ lockdep_is_held(&t->tcf_lock));
spin_unlock_bh(&t->tcf_lock);
tunnel_key_release_params(params_new);
if (goto_ch)
diff --git a/net/sched/act_vlan.c b/net/sched/act_vlan.c
index 9269d35..d5db65f 100644
--- a/net/sched/act_vlan.c
+++ b/net/sched/act_vlan.c
@@ -218,7 +218,7 @@ static int tcf_vlan_init(struct net *net, struct nlattr *nla,
spin_lock_bh(&v->tcf_lock);
goto_ch = tcf_action_set_ctrlact(*a, parm->action, goto_ch);
- rcu_swap_protected(v->vlan_p, p, lockdep_is_held(&v->tcf_lock));
+ p = rcu_replace(v->vlan_p, p, lockdep_is_held(&v->tcf_lock));
spin_unlock_bh(&v->tcf_lock);
if (goto_ch)
diff --git a/security/safesetid/securityfs.c b/security/safesetid/securityfs.c
index d568e17..3d5ee77 100644
--- a/security/safesetid/securityfs.c
+++ b/security/safesetid/securityfs.c
@@ -179,15 +179,16 @@ static ssize_t handle_policy_update(struct file *file,
* doesn't currently exist, just use a spinlock for now.
*/
mutex_lock(&policy_update_lock);
- rcu_swap_protected(safesetid_setuid_rules, pol,
- lockdep_is_held(&policy_update_lock));
+ pol = rcu_replace(safesetid_setuid_rules, pol,
+ lockdep_is_held(&policy_update_lock));
mutex_unlock(&policy_update_lock);
err = len;
out_free_buf:
kfree(buf);
out_free_pol:
- release_ruleset(pol);
+ if (pol)
+ release_ruleset(pol);
return err;
}
On Mon, 23 Sep 2019, Linus Torvalds wrote:
> Should we just remove safesetid again? It's not really maintained, and
> it's apparently not used. It was merged in March (with the first
> commit in January), and here we are at end of September and this
> happens.
My understanding is that SafeSetID is shipping in ChromeOS -- this was
part of the rationale for merging it.
--
James Morris
<[email protected]>
On Mon, Sep 23, 2019 at 4:35 PM James Morris <[email protected]> wrote:
>
> My understanding is that SafeSetID is shipping in ChromeOS -- this was
> part of the rationale for merging it.
Well, if even the developer didn't test it for two months, I don't
think "it's in upstream" makes any sense or difference.
Linus
On Mon, Sep 23, 2019 at 4:30 PM Paul E. McKenney <[email protected]> wrote:
>
> I pushed some (untested) commits out to the dev branch of -rcu, the
> overall effect of which is shown in the patch below. The series
> adds a new rcu_replace() to avoid confusion with swap(), replaces
> uses of rcu_swap_protected() with rcu_replace(), and finally removes
> rcu_swap_protected().
>
> Is this what you had in mind?
>
> Unless you tell me otherwise, I will assume that this change is important
> but not violently urgent. (As in not for the current merge window.)
Ack, looks good to me,
Linus
On Mon, Sep 23, 2019 at 5:45 PM Linus Torvalds
<[email protected]> wrote:
>
> On Mon, Sep 23, 2019 at 4:35 PM James Morris <[email protected]> wrote:
> >
> > My understanding is that SafeSetID is shipping in ChromeOS -- this was
> > part of the rationale for merging it.
>
> Well, if even the developer didn't test it for two months, I don't
> think "it's in upstream" makes any sense or difference.
>
> Linus
Yes, SafeSetID is shipping on Chrome OS, although I agree having that
bug in 5.3 without anyone noticing is bad. When Jann sent the last
round of patches for 5.3 he had tested the code and everything looked
good, although I unfortunately neglected to test it again after a
tweak to one of the patches, which of course broke stuff when the
patches ultimately went in.
Even though this is enabled in production for Chrome OS, none of the
Chrome OS devices are using version 5.3 yet, so it went unnoticed on
Chrome OS so far. In general the fact that a kernel feature is
shipping on Chrome OS isn't an up-to-date assurance that the feature
works in the most recent Linux release, as it would likely be months
(at least) from when a change makes it into the kernel until that
kernel release is ever run on a Chrome OS device (right now the most
recent kernel we ship on Chrome OS is 4.19, so I've had to backport
the SafeSetID stuff).
We've found this SafeSetID LSM to be pretty useful on Chrome OS, and
more use cases have popped up than we had in mind when writing it,
which suggests others would potentially find it useful as well. But I
understand for it to be useful to others it needs to be stable and
functional on every release. The best way I know of ensuring this is
for me to personally run the SafeSetID selftest (in
tools/testing/selftests/safesetid/) every release, regardless of
whether we make any changes to SafeSetID itself. Does this sound
sufficient or are there more formal guidelines/processes here that I'm
not aware of?
The pull request you sent on Wed, 18 Sep 2019 10:41:06 -0700:
> https://github.com/micah-morton/linux.git tags/safesetid-bugfix-5.4
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/1b5fb415442eb3ec946d48afe8c87b0f2fd42d7c
Thank you!
--
Deet-doot-dot, I am a bot.
https://korg.wiki.kernel.org/userdoc/prtracker
On Wed, Sep 18, 2019 at 10:41 AM Micah Morton <[email protected]> wrote:
>
> Fix for SafeSetID bug that was introduced in 5.3
So this seems to be a good fix, but the bug itself came from the fact that
rcu_swap_protected(..)
is so hard to read, and I don't see *why* it's so pointlessly hard to read.
Yes, we have some macros that change their arguments, but they have a
_reason_ to do so (ie they return two different values) and they tend
to be very special in other ways too.
But rcu_swap_protected() has no reason for it's odd semantics.
Looking at that 'handle_policy_update()' function, it's entirely
reasonable to think "pol cannot possibly be NULL". When I looked at
the fix patch, that was my initial reaction too, and it's probably the
reason Jann's commit 03638e62f55f ("LSM: SafeSetID: rewrite userspace
API to atomic updates") had that bug to begin with.
I don't see the original discussion at all, it's not on
Linux-Security-Module for some reason, so I can't tell when/if the
NULL pointer test got deleted.
Anyway, this bug would likely had been avoided if rcu_swap_protected()
just returned the old pointer instead of changing the argument.
There are only a handful or users of that macro, maybe this could be fixed?
Adding some of the RCU parties to the participants..
Also, the commit message for this fix was a mess, I feel. It says
"SafeSetID: Stop releasing uninitialized ruleset", but the ruleset it
releases is perfectly initialized. It just might be NULL because it
doesn't _exist_.
Linus
On Mon, Sep 23, 2019 at 8:31 PM Micah Morton <[email protected]> wrote:
>
> The best way I know of ensuring this is
> for me to personally run the SafeSetID selftest (in
> tools/testing/selftests/safesetid/) every release, regardless of
> whether we make any changes to SafeSetID itself. Does this sound
> sufficient or are there more formal guidelines/processes here that I'm
> not aware of?
I think that would help, but I wopuld also hope that somebody actually
runs Chromium / Chrome OS with a modern kernel.
Even if *standard* device installs don't end up having recent kernels,
I would assume there are people who are testing development setups?
Linus
* Paul E. McKenney <[email protected]> wrote:
> --- a/include/linux/rcupdate.h
> +++ b/include/linux/rcupdate.h
> @@ -383,20 +383,22 @@ do { \
> } while (0)
>
> /**
> - * rcu_swap_protected() - swap an RCU and a regular pointer
> - * @rcu_ptr: RCU pointer
> + * rcu_replace() - replace an RCU pointer, returning its old value
> + * @rcu_ptr: RCU pointer, whose old value is returned
> * @ptr: regular pointer
> - * @c: the conditions under which the dereference will take place
> + * @c: the lockdep conditions under which the dereference will take place
> *
> - * Perform swap(@rcu_ptr, @ptr) where @rcu_ptr is an RCU-annotated pointer and
> - * @c is the argument that is passed to the rcu_dereference_protected() call
> - * used to read that pointer.
> + * Perform a replacement, where @rcu_ptr is an RCU-annotated
> + * pointer and @c is the lockdep argument that is passed to the
> + * rcu_dereference_protected() call used to read that pointer. The old
> + * value of @rcu_ptr is returned, and @rcu_ptr is set to @ptr.
> */
> -#define rcu_swap_protected(rcu_ptr, ptr, c) do { \
> +#define rcu_replace(rcu_ptr, ptr, c) \
> +({ \
> typeof(ptr) __tmp = rcu_dereference_protected((rcu_ptr), (c)); \
> rcu_assign_pointer((rcu_ptr), (ptr)); \
> - (ptr) = __tmp; \
> -} while (0)
> + __tmp; \
> +})
One small suggestion, would it make sense to name it "rcu_replace_pointer()"?
This would make it fit into the pointer handling family of RCU functions:
rcu_assign_pointer(), rcu_access_pointer(), RCU_INIT_POINTER() et al?
rcu_swap() would also look a bit weird if used in MM code. ;-)
Thanks,
Ingo
On Mon, Oct 21, 2019 at 08:58:11AM +0200, Ingo Molnar wrote:
>
> * Paul E. McKenney <[email protected]> wrote:
>
> > --- a/include/linux/rcupdate.h
> > +++ b/include/linux/rcupdate.h
> > @@ -383,20 +383,22 @@ do { \
> > } while (0)
> >
> > /**
> > - * rcu_swap_protected() - swap an RCU and a regular pointer
> > - * @rcu_ptr: RCU pointer
> > + * rcu_replace() - replace an RCU pointer, returning its old value
> > + * @rcu_ptr: RCU pointer, whose old value is returned
> > * @ptr: regular pointer
> > - * @c: the conditions under which the dereference will take place
> > + * @c: the lockdep conditions under which the dereference will take place
> > *
> > - * Perform swap(@rcu_ptr, @ptr) where @rcu_ptr is an RCU-annotated pointer and
> > - * @c is the argument that is passed to the rcu_dereference_protected() call
> > - * used to read that pointer.
> > + * Perform a replacement, where @rcu_ptr is an RCU-annotated
> > + * pointer and @c is the lockdep argument that is passed to the
> > + * rcu_dereference_protected() call used to read that pointer. The old
> > + * value of @rcu_ptr is returned, and @rcu_ptr is set to @ptr.
> > */
> > -#define rcu_swap_protected(rcu_ptr, ptr, c) do { \
> > +#define rcu_replace(rcu_ptr, ptr, c) \
> > +({ \
> > typeof(ptr) __tmp = rcu_dereference_protected((rcu_ptr), (c)); \
> > rcu_assign_pointer((rcu_ptr), (ptr)); \
> > - (ptr) = __tmp; \
> > -} while (0)
> > + __tmp; \
> > +})
>
> One small suggestion, would it make sense to name it "rcu_replace_pointer()"?
>
> This would make it fit into the pointer handling family of RCU functions:
> rcu_assign_pointer(), rcu_access_pointer(), RCU_INIT_POINTER() et al?
Easy enough to make the change. I will do that tomorrow and test over
the following night.
> rcu_swap() would also look a bit weird if used in MM code. ;-)
How much RCU swap should we configure on this system? About same amount
as reader-writer swap! ;-)
Thanx, Paul