* Re: [PATCH v2] include/uapi/linux/xfrm.h: Fix XFRM_MSG_MAPPING ABI breakage
2021-09-12 12:22 [PATCH v2] include/uapi/linux/xfrm.h: Fix XFRM_MSG_MAPPING ABI breakage Eugene Syromiatnikov
@ 2021-09-13 6:54 ` Antony Antony
2021-09-13 7:16 ` Ondrej Mosnacek
` (2 subsequent siblings)
3 siblings, 0 replies; 7+ messages in thread
From: Antony Antony @ 2021-09-13 6:54 UTC (permalink / raw)
To: Eugene Syromiatnikov
Cc: Steffen Klassert, Herbert Xu, David S. Miller, Antony Antony,
Christian Langrock, Nicolas Dichtel, selinux, Paul Moore,
Stephen Smalley, Eric Paris, netdev, linux-kernel,
Dmitry V. Levin, linux-api
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
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v2] include/uapi/linux/xfrm.h: Fix XFRM_MSG_MAPPING ABI breakage
2021-09-12 12:22 [PATCH v2] include/uapi/linux/xfrm.h: Fix XFRM_MSG_MAPPING ABI breakage Eugene Syromiatnikov
2021-09-13 6:54 ` Antony Antony
@ 2021-09-13 7:16 ` Ondrej Mosnacek
2021-09-13 10:23 ` Eugene Syromiatnikov
2021-09-14 7:50 ` Nicolas Dichtel
2021-09-15 8:51 ` Steffen Klassert
3 siblings, 1 reply; 7+ messages in thread
From: Ondrej Mosnacek @ 2021-09-13 7:16 UTC (permalink / raw)
To: Eugene Syromiatnikov
Cc: Steffen Klassert, Herbert Xu, David S. Miller, Antony Antony,
Christian Langrock, Nicolas Dichtel, SElinux list, Paul Moore,
Stephen Smalley, Eric Paris, network dev,
Linux kernel mailing list, Dmitry V. Levin, Linux API
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.
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v2] include/uapi/linux/xfrm.h: Fix XFRM_MSG_MAPPING ABI breakage
2021-09-13 7:16 ` Ondrej Mosnacek
@ 2021-09-13 10:23 ` Eugene Syromiatnikov
2021-09-13 13:50 ` Ondrej Mosnacek
0 siblings, 1 reply; 7+ messages in thread
From: Eugene Syromiatnikov @ 2021-09-13 10:23 UTC (permalink / raw)
To: Ondrej Mosnacek
Cc: Steffen Klassert, Herbert Xu, David S. Miller, Antony Antony,
Christian Langrock, Nicolas Dichtel, SElinux list, Paul Moore,
Stephen Smalley, Eric Paris, network dev,
Linux kernel mailing list, Dmitry V. Levin, Linux API
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] include/uapi/linux/xfrm.h: Fix XFRM_MSG_MAPPING ABI breakage
2021-09-13 10:23 ` Eugene Syromiatnikov
@ 2021-09-13 13:50 ` Ondrej Mosnacek
0 siblings, 0 replies; 7+ messages in thread
From: Ondrej Mosnacek @ 2021-09-13 13:50 UTC (permalink / raw)
To: Eugene Syromiatnikov
Cc: Steffen Klassert, Herbert Xu, David S. Miller, Antony Antony,
Christian Langrock, Nicolas Dichtel, SElinux list, Paul Moore,
Stephen Smalley, Eric Paris, network dev,
Linux kernel mailing list, Dmitry V. Levin, Linux API
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] include/uapi/linux/xfrm.h: Fix XFRM_MSG_MAPPING ABI breakage
2021-09-12 12:22 [PATCH v2] include/uapi/linux/xfrm.h: Fix XFRM_MSG_MAPPING ABI breakage Eugene Syromiatnikov
2021-09-13 6:54 ` Antony Antony
2021-09-13 7:16 ` Ondrej Mosnacek
@ 2021-09-14 7:50 ` Nicolas Dichtel
2021-09-15 8:51 ` Steffen Klassert
3 siblings, 0 replies; 7+ messages in thread
From: Nicolas Dichtel @ 2021-09-14 7:50 UTC (permalink / raw)
To: Eugene Syromiatnikov, Steffen Klassert, Herbert Xu,
David S. Miller, Antony Antony, Christian Langrock
Cc: selinux, Paul Moore, Stephen Smalley, Eric Paris, netdev,
linux-kernel, Dmitry V. Levin, linux-api
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>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v2] include/uapi/linux/xfrm.h: Fix XFRM_MSG_MAPPING ABI breakage
2021-09-12 12:22 [PATCH v2] include/uapi/linux/xfrm.h: Fix XFRM_MSG_MAPPING ABI breakage Eugene Syromiatnikov
` (2 preceding siblings ...)
2021-09-14 7:50 ` Nicolas Dichtel
@ 2021-09-15 8:51 ` Steffen Klassert
3 siblings, 0 replies; 7+ messages in thread
From: Steffen Klassert @ 2021-09-15 8:51 UTC (permalink / raw)
To: Eugene Syromiatnikov
Cc: Herbert Xu, David S. Miller, Antony Antony, Christian Langrock,
Nicolas Dichtel, selinux, Paul Moore, Stephen Smalley,
Eric Paris, netdev, linux-kernel, Dmitry V. Levin, linux-api
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!
^ permalink raw reply [flat|nested] 7+ messages in thread