| Message ID | 20210912122234.GA22469@asgard.redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] include/uapi/linux/xfrm.h: Fix XFRM_MSG_MAPPING ABI breakage | expand |
Thanks! Acked-by: Antony Antony <antony.antony@secunet.com> -antony On Sun, Sep 12, 2021 at 14:22:34 +0200, Eugene Syromiatnikov wrote: > Commit 2d151d39073a ("xfrm: Add possibility to set the default to block > if we have no policy") broke ABI by changing the value of the XFRM_MSG_MAPPING > enum item, thus also evading the build-time check > in security/selinux/nlmsgtab.c:selinux_nlmsg_lookup for presence of proper > security permission checks in nlmsg_xfrm_perms. Fix it by placing > XFRM_MSG_SETDEFAULT/XFRM_MSG_GETDEFAULT to the end of the enum, right before > __XFRM_MSG_MAX, and updating the nlmsg_xfrm_perms accordingly. > > Fixes: 2d151d39073a ("xfrm: Add possibility to set the default to block if we have no policy") > References: https://lore.kernel.org/netdev/20210901151402.GA2557@altlinux.org/ > Signed-off-by: Eugene Syromiatnikov <esyr@redhat.com> > --- > v2: > - Updated SELinux nlmsg_xfrm_perms permissions table and selinux_nlmsg_lookup > build-time check accordingly. > > v1: https://lore.kernel.org/lkml/20210901153407.GA20446@asgard.redhat.com/ > --- > include/uapi/linux/xfrm.h | 6 +++--- > security/selinux/nlmsgtab.c | 4 +++- > 2 files changed, 6 insertions(+), 4 deletions(-) > > diff --git a/include/uapi/linux/xfrm.h b/include/uapi/linux/xfrm.h > index b96c1ea..26f456b1 100644 > --- a/include/uapi/linux/xfrm.h > +++ b/include/uapi/linux/xfrm.h > @@ -213,13 +213,13 @@ enum { > XFRM_MSG_GETSPDINFO, > #define XFRM_MSG_GETSPDINFO XFRM_MSG_GETSPDINFO > > + XFRM_MSG_MAPPING, > +#define XFRM_MSG_MAPPING XFRM_MSG_MAPPING > + > XFRM_MSG_SETDEFAULT, > #define XFRM_MSG_SETDEFAULT XFRM_MSG_SETDEFAULT > XFRM_MSG_GETDEFAULT, > #define XFRM_MSG_GETDEFAULT XFRM_MSG_GETDEFAULT > - > - XFRM_MSG_MAPPING, > -#define XFRM_MSG_MAPPING XFRM_MSG_MAPPING > __XFRM_MSG_MAX > }; > #define XFRM_MSG_MAX (__XFRM_MSG_MAX - 1) > diff --git a/security/selinux/nlmsgtab.c b/security/selinux/nlmsgtab.c > index d59276f..94ea2a8 100644 > --- a/security/selinux/nlmsgtab.c > +++ b/security/selinux/nlmsgtab.c > @@ -126,6 +126,8 @@ static const struct nlmsg_perm nlmsg_xfrm_perms[] = > { XFRM_MSG_NEWSPDINFO, NETLINK_XFRM_SOCKET__NLMSG_WRITE }, > { XFRM_MSG_GETSPDINFO, NETLINK_XFRM_SOCKET__NLMSG_READ }, > { XFRM_MSG_MAPPING, NETLINK_XFRM_SOCKET__NLMSG_READ }, > + { XFRM_MSG_SETDEFAULT, NETLINK_XFRM_SOCKET__NLMSG_WRITE }, > + { XFRM_MSG_GETDEFAULT, NETLINK_XFRM_SOCKET__NLMSG_READ }, > }; > > static const struct nlmsg_perm nlmsg_audit_perms[] = > @@ -189,7 +191,7 @@ int selinux_nlmsg_lookup(u16 sclass, u16 nlmsg_type, u32 *perm) > * structures at the top of this file with the new mappings > * before updating the BUILD_BUG_ON() macro! > */ > - BUILD_BUG_ON(XFRM_MSG_MAX != XFRM_MSG_MAPPING); > + BUILD_BUG_ON(XFRM_MSG_MAX != XFRM_MSG_GETDEFAULT); > err = nlmsg_perm(nlmsg_type, perm, nlmsg_xfrm_perms, > sizeof(nlmsg_xfrm_perms)); > break; > -- > 2.1.4 >
Hi, On Sun, Sep 12, 2021 at 2:23 PM Eugene Syromiatnikov <esyr@redhat.com> wrote: > Commit 2d151d39073a ("xfrm: Add possibility to set the default to block > if we have no policy") broke ABI by changing the value of the XFRM_MSG_MAPPING > enum item, thus also evading the build-time check > in security/selinux/nlmsgtab.c:selinux_nlmsg_lookup for presence of proper > security permission checks in nlmsg_xfrm_perms. Fix it by placing > XFRM_MSG_SETDEFAULT/XFRM_MSG_GETDEFAULT to the end of the enum, right before > __XFRM_MSG_MAX, and updating the nlmsg_xfrm_perms accordingly. > > Fixes: 2d151d39073a ("xfrm: Add possibility to set the default to block if we have no policy") > References: https://lore.kernel.org/netdev/20210901151402.GA2557@altlinux.org/ > Signed-off-by: Eugene Syromiatnikov <esyr@redhat.com> > --- > v2: > - Updated SELinux nlmsg_xfrm_perms permissions table and selinux_nlmsg_lookup > build-time check accordingly. > > v1: https://lore.kernel.org/lkml/20210901153407.GA20446@asgard.redhat.com/ > --- > include/uapi/linux/xfrm.h | 6 +++--- > security/selinux/nlmsgtab.c | 4 +++- > 2 files changed, 6 insertions(+), 4 deletions(-) > > diff --git a/include/uapi/linux/xfrm.h b/include/uapi/linux/xfrm.h > index b96c1ea..26f456b1 100644 > --- a/include/uapi/linux/xfrm.h > +++ b/include/uapi/linux/xfrm.h > @@ -213,13 +213,13 @@ enum { > XFRM_MSG_GETSPDINFO, > #define XFRM_MSG_GETSPDINFO XFRM_MSG_GETSPDINFO > > + XFRM_MSG_MAPPING, > +#define XFRM_MSG_MAPPING XFRM_MSG_MAPPING > + > XFRM_MSG_SETDEFAULT, > #define XFRM_MSG_SETDEFAULT XFRM_MSG_SETDEFAULT > XFRM_MSG_GETDEFAULT, > #define XFRM_MSG_GETDEFAULT XFRM_MSG_GETDEFAULT > - > - XFRM_MSG_MAPPING, > -#define XFRM_MSG_MAPPING XFRM_MSG_MAPPING Perhaps it would be a good idea to put a comment here to make it less likely that this repeats in the future. Something like: /* IMPORTANT: Only insert new entries right above this line, otherwise you break ABI! */ > __XFRM_MSG_MAX > }; > #define XFRM_MSG_MAX (__XFRM_MSG_MAX - 1) > diff --git a/security/selinux/nlmsgtab.c b/security/selinux/nlmsgtab.c > index d59276f..94ea2a8 100644 > --- a/security/selinux/nlmsgtab.c > +++ b/security/selinux/nlmsgtab.c > @@ -126,6 +126,8 @@ static const struct nlmsg_perm nlmsg_xfrm_perms[] = > { XFRM_MSG_NEWSPDINFO, NETLINK_XFRM_SOCKET__NLMSG_WRITE }, > { XFRM_MSG_GETSPDINFO, NETLINK_XFRM_SOCKET__NLMSG_READ }, > { XFRM_MSG_MAPPING, NETLINK_XFRM_SOCKET__NLMSG_READ }, > + { XFRM_MSG_SETDEFAULT, NETLINK_XFRM_SOCKET__NLMSG_WRITE }, > + { XFRM_MSG_GETDEFAULT, NETLINK_XFRM_SOCKET__NLMSG_READ }, > }; > > static const struct nlmsg_perm nlmsg_audit_perms[] = > @@ -189,7 +191,7 @@ int selinux_nlmsg_lookup(u16 sclass, u16 nlmsg_type, u32 *perm) > * structures at the top of this file with the new mappings > * before updating the BUILD_BUG_ON() macro! > */ > - BUILD_BUG_ON(XFRM_MSG_MAX != XFRM_MSG_MAPPING); > + BUILD_BUG_ON(XFRM_MSG_MAX != XFRM_MSG_GETDEFAULT); > err = nlmsg_perm(nlmsg_type, perm, nlmsg_xfrm_perms, > sizeof(nlmsg_xfrm_perms)); > break; > -- > 2.1.4 > -- Ondrej Mosnacek Software Engineer, Linux Security - SELinux kernel Red Hat, Inc.
On Mon, Sep 13, 2021 at 09:16:39AM +0200, Ondrej Mosnacek wrote: > Perhaps it would be a good idea to put a comment here to make it less > likely that this repeats in the future. Something like: > > /* IMPORTANT: Only insert new entries right above this line, otherwise > you break ABI! */ Well, this statement is true for (almost) every UAPI-exposed enum, and netlink is vast and relies on enums heavily. I think it is already mentioned somewhere in the documentation, and in the end it falls on the shoulders of the maintainers—to pay additional attention to UAPI changes.
On Mon, Sep 13, 2021 at 12:23 PM Eugene Syromiatnikov <esyr@redhat.com> wrote: > On Mon, Sep 13, 2021 at 09:16:39AM +0200, Ondrej Mosnacek wrote: > > Perhaps it would be a good idea to put a comment here to make it less > > likely that this repeats in the future. Something like: > > > > /* IMPORTANT: Only insert new entries right above this line, otherwise > > you break ABI! */ > > Well, this statement is true for (almost) every UAPI-exposed enum, and > netlink is vast and relies on enums heavily. I think it is already > mentioned somewhere in the documentation, and in the end it falls on the > shoulders of the maintainers—to pay additional attention to UAPI changes. Ok, fair enough. -- Ondrej Mosnacek Software Engineer, Linux Security - SELinux kernel Red Hat, Inc.
Le 12/09/2021 à 14:22, Eugene Syromiatnikov a écrit : > Commit 2d151d39073a ("xfrm: Add possibility to set the default to block > if we have no policy") broke ABI by changing the value of the XFRM_MSG_MAPPING > enum item, thus also evading the build-time check > in security/selinux/nlmsgtab.c:selinux_nlmsg_lookup for presence of proper > security permission checks in nlmsg_xfrm_perms. Fix it by placing > XFRM_MSG_SETDEFAULT/XFRM_MSG_GETDEFAULT to the end of the enum, right before > __XFRM_MSG_MAX, and updating the nlmsg_xfrm_perms accordingly. > > Fixes: 2d151d39073a ("xfrm: Add possibility to set the default to block if we have no policy") > References: https://lore.kernel.org/netdev/20210901151402.GA2557@altlinux.org/ > Signed-off-by: Eugene Syromiatnikov <esyr@redhat.com> Acked-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
On Sun, Sep 12, 2021 at 02:22:34PM +0200, Eugene Syromiatnikov wrote: > Commit 2d151d39073a ("xfrm: Add possibility to set the default to block > if we have no policy") broke ABI by changing the value of the XFRM_MSG_MAPPING > enum item, thus also evading the build-time check > in security/selinux/nlmsgtab.c:selinux_nlmsg_lookup for presence of proper > security permission checks in nlmsg_xfrm_perms. Fix it by placing > XFRM_MSG_SETDEFAULT/XFRM_MSG_GETDEFAULT to the end of the enum, right before > __XFRM_MSG_MAX, and updating the nlmsg_xfrm_perms accordingly. > > Fixes: 2d151d39073a ("xfrm: Add possibility to set the default to block if we have no policy") > References: https://lore.kernel.org/netdev/20210901151402.GA2557@altlinux.org/ > Signed-off-by: Eugene Syromiatnikov <esyr@redhat.com> Applied, thanks a lot Eugene!
diff --git a/include/uapi/linux/xfrm.h b/include/uapi/linux/xfrm.h index b96c1ea..26f456b1 100644 --- a/include/uapi/linux/xfrm.h +++ b/include/uapi/linux/xfrm.h @@ -213,13 +213,13 @@ enum { XFRM_MSG_GETSPDINFO, #define XFRM_MSG_GETSPDINFO XFRM_MSG_GETSPDINFO + XFRM_MSG_MAPPING, +#define XFRM_MSG_MAPPING XFRM_MSG_MAPPING + XFRM_MSG_SETDEFAULT, #define XFRM_MSG_SETDEFAULT XFRM_MSG_SETDEFAULT XFRM_MSG_GETDEFAULT, #define XFRM_MSG_GETDEFAULT XFRM_MSG_GETDEFAULT - - XFRM_MSG_MAPPING, -#define XFRM_MSG_MAPPING XFRM_MSG_MAPPING __XFRM_MSG_MAX }; #define XFRM_MSG_MAX (__XFRM_MSG_MAX - 1) diff --git a/security/selinux/nlmsgtab.c b/security/selinux/nlmsgtab.c index d59276f..94ea2a8 100644 --- a/security/selinux/nlmsgtab.c +++ b/security/selinux/nlmsgtab.c @@ -126,6 +126,8 @@ static const struct nlmsg_perm nlmsg_xfrm_perms[] = { XFRM_MSG_NEWSPDINFO, NETLINK_XFRM_SOCKET__NLMSG_WRITE }, { XFRM_MSG_GETSPDINFO, NETLINK_XFRM_SOCKET__NLMSG_READ }, { XFRM_MSG_MAPPING, NETLINK_XFRM_SOCKET__NLMSG_READ }, + { XFRM_MSG_SETDEFAULT, NETLINK_XFRM_SOCKET__NLMSG_WRITE }, + { XFRM_MSG_GETDEFAULT, NETLINK_XFRM_SOCKET__NLMSG_READ }, }; static const struct nlmsg_perm nlmsg_audit_perms[] = @@ -189,7 +191,7 @@ int selinux_nlmsg_lookup(u16 sclass, u16 nlmsg_type, u32 *perm) * structures at the top of this file with the new mappings * before updating the BUILD_BUG_ON() macro! */ - BUILD_BUG_ON(XFRM_MSG_MAX != XFRM_MSG_MAPPING); + BUILD_BUG_ON(XFRM_MSG_MAX != XFRM_MSG_GETDEFAULT); err = nlmsg_perm(nlmsg_type, perm, nlmsg_xfrm_perms, sizeof(nlmsg_xfrm_perms)); break;
Commit 2d151d39073a ("xfrm: Add possibility to set the default to block if we have no policy") broke ABI by changing the value of the XFRM_MSG_MAPPING enum item, thus also evading the build-time check in security/selinux/nlmsgtab.c:selinux_nlmsg_lookup for presence of proper security permission checks in nlmsg_xfrm_perms. Fix it by placing XFRM_MSG_SETDEFAULT/XFRM_MSG_GETDEFAULT to the end of the enum, right before __XFRM_MSG_MAX, and updating the nlmsg_xfrm_perms accordingly. Fixes: 2d151d39073a ("xfrm: Add possibility to set the default to block if we have no policy") References: https://lore.kernel.org/netdev/20210901151402.GA2557@altlinux.org/ Signed-off-by: Eugene Syromiatnikov <esyr@redhat.com> --- v2: - Updated SELinux nlmsg_xfrm_perms permissions table and selinux_nlmsg_lookup build-time check accordingly. v1: https://lore.kernel.org/lkml/20210901153407.GA20446@asgard.redhat.com/ --- include/uapi/linux/xfrm.h | 6 +++--- security/selinux/nlmsgtab.c | 4 +++- 2 files changed, 6 insertions(+), 4 deletions(-)