| Message ID | 20220518092137.141626-1-gongruiqi1@huawei.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | selinux: add __randomize_layout to selinux_audit_data | expand |
On Tue, May 17, 2022 at 9:21 PM GONG, Ruiqi <gongruiqi1@huawei.com> wrote: > > Randomize the layout of struct selinux_audit_data as suggested in [1], > since it contains a pointer to struct selinux_state, an already > randomized strucure. > > [1]: https://github.com/KSPP/linux/issues/188 > > Signed-off-by: GONG, Ruiqi <gongruiqi1@huawei.com> > --- > security/selinux/include/avc.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/security/selinux/include/avc.h b/security/selinux/include/avc.h > index 2b372f98f2d7..5525b94fd266 100644 > --- a/security/selinux/include/avc.h > +++ b/security/selinux/include/avc.h > @@ -53,7 +53,7 @@ struct selinux_audit_data { > u32 denied; > int result; > struct selinux_state *state; > -}; > +} __randomize_layout; I'll apologize in advance for the stupid question, but does __randomize_layout result in any problems when the struct is used in a trace event? (see include/trace/events/avc.h)
On 2022/05/18 9:39, Paul Moore wrote: > On Tue, May 17, 2022 at 9:21 PM GONG, Ruiqi <gongruiqi1@huawei.com> wrote: >> >> Randomize the layout of struct selinux_audit_data as suggested in [1], >> since it contains a pointer to struct selinux_state, an already >> randomized strucure. >> >> [1]: https://github.com/KSPP/linux/issues/188 >> >> Signed-off-by: GONG, Ruiqi <gongruiqi1@huawei.com> >> --- >> security/selinux/include/avc.h | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/security/selinux/include/avc.h b/security/selinux/include/avc.h >> index 2b372f98f2d7..5525b94fd266 100644 >> --- a/security/selinux/include/avc.h >> +++ b/security/selinux/include/avc.h >> @@ -53,7 +53,7 @@ struct selinux_audit_data { >> u32 denied; >> int result; >> struct selinux_state *state; >> -}; >> +} __randomize_layout; > > I'll apologize in advance for the stupid question, but does Not at all :) > __randomize_layout result in any problems when the struct is used in a > trace event? (see include/trace/events/avc.h) > No, as least it doesn't in the testing I did. I believe we can use the struct tagged with __randomize_layout as normal except that 1) it should be initialized with a designated initializer, and 2) pointers to this type can't be cast to/from pointers to another type. Other operations like dereferencing members of the struct (as in include/trace/events/avc.h) shouldn't be a problem. I did a testing to the patch on a qemu vm by running the selinux testsuite with tracing events "avc:selinux_audited" enabled. The testsuite completed successfully and from the tracing log I saw nothing abnormal with my bare eyes. You can do more testing if you want or you have other ideas of how to do so ;)
On Wed, May 18, 2022 at 2:53 AM Gong Ruiqi <gongruiqi1@huawei.com> wrote: > On 2022/05/18 9:39, Paul Moore wrote: > > On Tue, May 17, 2022 at 9:21 PM GONG, Ruiqi <gongruiqi1@huawei.com> wrote: > >> > >> Randomize the layout of struct selinux_audit_data as suggested in [1], > >> since it contains a pointer to struct selinux_state, an already > >> randomized strucure. > >> > >> [1]: https://github.com/KSPP/linux/issues/188 > >> > >> Signed-off-by: GONG, Ruiqi <gongruiqi1@huawei.com> > >> --- > >> security/selinux/include/avc.h | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/security/selinux/include/avc.h b/security/selinux/include/avc.h > >> index 2b372f98f2d7..5525b94fd266 100644 > >> --- a/security/selinux/include/avc.h > >> +++ b/security/selinux/include/avc.h > >> @@ -53,7 +53,7 @@ struct selinux_audit_data { > >> u32 denied; > >> int result; > >> struct selinux_state *state; > >> -}; > >> +} __randomize_layout; > > > > I'll apologize in advance for the stupid question, but does > > Not at all :) > > > __randomize_layout result in any problems when the struct is used in a > > trace event? (see include/trace/events/avc.h) > > No, as least it doesn't in the testing I did. I believe we can use the > struct tagged with __randomize_layout as normal except that 1) it should > be initialized with a designated initializer, and 2) pointers to this > type can't be cast to/from pointers to another type. Other operations > like dereferencing members of the struct (as in > include/trace/events/avc.h) shouldn't be a problem. > > I did a testing to the patch on a qemu vm by running the selinux > testsuite with tracing events "avc:selinux_audited" enabled. The > testsuite completed successfully and from the tracing log I saw nothing > abnormal with my bare eyes. You can do more testing if you want or you > have other ideas of how to do so ;) That's great, thanks for verifying this. I was aware of the other restrictions but wasn't sure about tracing. Now I know :) It's too late to go into selinux/next for this dev cycle, but I'll queue this up for selinux/next once the merge window closes. Thanks again!
On Wed, May 18, 2022 at 12:03 PM Paul Moore <paul@paul-moore.com> wrote: > > On Wed, May 18, 2022 at 2:53 AM Gong Ruiqi <gongruiqi1@huawei.com> wrote: > > On 2022/05/18 9:39, Paul Moore wrote: > > > On Tue, May 17, 2022 at 9:21 PM GONG, Ruiqi <gongruiqi1@huawei.com> wrote: > > >> > > >> Randomize the layout of struct selinux_audit_data as suggested in [1], > > >> since it contains a pointer to struct selinux_state, an already > > >> randomized strucure. > > >> > > >> [1]: https://github.com/KSPP/linux/issues/188 > > >> > > >> Signed-off-by: GONG, Ruiqi <gongruiqi1@huawei.com> > > >> --- > > >> security/selinux/include/avc.h | 2 +- > > >> 1 file changed, 1 insertion(+), 1 deletion(-) > > >> > > >> diff --git a/security/selinux/include/avc.h b/security/selinux/include/avc.h > > >> index 2b372f98f2d7..5525b94fd266 100644 > > >> --- a/security/selinux/include/avc.h > > >> +++ b/security/selinux/include/avc.h > > >> @@ -53,7 +53,7 @@ struct selinux_audit_data { > > >> u32 denied; > > >> int result; > > >> struct selinux_state *state; > > >> -}; > > >> +} __randomize_layout; > > > > > > I'll apologize in advance for the stupid question, but does > > > > Not at all :) > > > > > __randomize_layout result in any problems when the struct is used in a > > > trace event? (see include/trace/events/avc.h) > > > > No, as least it doesn't in the testing I did. I believe we can use the > > struct tagged with __randomize_layout as normal except that 1) it should > > be initialized with a designated initializer, and 2) pointers to this > > type can't be cast to/from pointers to another type. Other operations > > like dereferencing members of the struct (as in > > include/trace/events/avc.h) shouldn't be a problem. > > > > I did a testing to the patch on a qemu vm by running the selinux > > testsuite with tracing events "avc:selinux_audited" enabled. The > > testsuite completed successfully and from the tracing log I saw nothing > > abnormal with my bare eyes. You can do more testing if you want or you > > have other ideas of how to do so ;) > > That's great, thanks for verifying this. I was aware of the other > restrictions but wasn't sure about tracing. Now I know :) > > It's too late to go into selinux/next for this dev cycle, but I'll > queue this up for selinux/next once the merge window closes. I just merged this into my local selinux/next branch and I'll be pushing it up to the mail selinux tree later today - thanks!
diff --git a/security/selinux/include/avc.h b/security/selinux/include/avc.h index 2b372f98f2d7..5525b94fd266 100644 --- a/security/selinux/include/avc.h +++ b/security/selinux/include/avc.h @@ -53,7 +53,7 @@ struct selinux_audit_data { u32 denied; int result; struct selinux_state *state; -}; +} __randomize_layout; /* * AVC operations
Randomize the layout of struct selinux_audit_data as suggested in [1], since it contains a pointer to struct selinux_state, an already randomized strucure. [1]: https://github.com/KSPP/linux/issues/188 Signed-off-by: GONG, Ruiqi <gongruiqi1@huawei.com> --- security/selinux/include/avc.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)