[PATCH v2 1/2] ipe: return -ESTALE instead of -EINVAL on update when new policy has a lower version

[PATCH v2 1/2] ipe: return -ESTALE instead of -EINVAL on update when new policy has a lower version

[luca.boccassi]

From: Luca Boccassi <bluca@debian.org>

When loading policies in userspace we want a recognizable error when an
update attempts to use an old policy, as that is an error that needs
to be treated differently from an invalid policy. Use -ESTALE as it is
clear enough for an update mechanism.

Signed-off-by: Luca Boccassi <bluca@debian.org>
Reviewed-by: Serge Hallyn <serge@hallyn.com>
---
 security/ipe/policy.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/security/ipe/policy.c b/security/ipe/policy.c
index bf5aa97911e1..3a0069c6d5af 100644
--- a/security/ipe/policy.c
+++ b/security/ipe/policy.c
@@ -107,7 +107,7 @@ int ipe_update_policy(struct inode *root, const char *text, size_t textlen,
 	}
 
 	if (ver_to_u64(old) > ver_to_u64(new)) {
-		rc = -EINVAL;
+		rc = -ESTALE;
 		goto err;
 	}
 
-- 
2.39.5

[PATCH v2 2/2] ipe: also reject policy updates with the same version

[luca.boccassi]

From: Luca Boccassi <bluca@debian.org>

Currently IPE accepts an update that has the same version as the policy
being updated, but it doesn't make it a no-op nor it checks that the
old and new policyes are the same. So it is possible to change the
content of a policy, without changing its version. This is very
confusing from userspace when managing policies.
Instead change the update logic to reject updates that have the same
version with ESTALE, as that is much clearer and intuitive behaviour.

Signed-off-by: Luca Boccassi <bluca@debian.org>
Reviewed-by: Serge Hallyn <serge@hallyn.com>
---
v2: also update documentation

 Documentation/admin-guide/LSM/ipe.rst | 2 +-
 security/ipe/policy.c                 | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/admin-guide/LSM/ipe.rst b/Documentation/admin-guide/LSM/ipe.rst
index 47323494d119..7cd04d3261ad 100644
--- a/Documentation/admin-guide/LSM/ipe.rst
+++ b/Documentation/admin-guide/LSM/ipe.rst
@@ -269,7 +269,7 @@ in the kernel. This file is write-only and accepts a PKCS#7 signed
 policy. Two checks will always be performed on this policy: First, the
 ``policy_names`` must match with the updated version and the existing
 version. Second the updated policy must have a policy version greater than
-or equal to the currently-running version. This is to prevent rollback attacks.
+to the currently-running version. This is to prevent rollback attacks.
 
 The ``delete`` file is used to remove a policy that is no longer needed.
 This file is write-only and accepts a value of ``1`` to delete the policy.
diff --git a/security/ipe/policy.c b/security/ipe/policy.c
index 3a0069c6d5af..45f7d6a0ed23 100644
--- a/security/ipe/policy.c
+++ b/security/ipe/policy.c
@@ -106,7 +106,7 @@ int ipe_update_policy(struct inode *root, const char *text, size_t textlen,
 		goto err;
 	}
 
-	if (ver_to_u64(old) > ver_to_u64(new)) {
+	if (ver_to_u64(old) >= ver_to_u64(new)) {
 		rc = -ESTALE;
 		goto err;
 	}
-- 
2.39.5

Re: [PATCH v2 2/2] ipe: also reject policy updates with the same version

[Fan Wu]

On 9/25/2024 1:42 PM, luca.boccassi@gmail.com wrote:
> From: Luca Boccassi <bluca@debian.org>
> 
> Currently IPE accepts an update that has the same version as the policy
> being updated, but it doesn't make it a no-op nor it checks that the
> old and new policyes are the same. So it is possible to change the
> content of a policy, without changing its version. This is very
> confusing from userspace when managing policies.
> Instead change the update logic to reject updates that have the same
> version with ESTALE, as that is much clearer and intuitive behaviour.
> 
> Signed-off-by: Luca Boccassi <bluca@debian.org>
> Reviewed-by: Serge Hallyn <serge@hallyn.com>
> ---
> v2: also update documentation
> 
>   Documentation/admin-guide/LSM/ipe.rst | 2 +-
>   security/ipe/policy.c                 | 2 +-
>   2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/admin-guide/LSM/ipe.rst b/Documentation/admin-guide/LSM/ipe.rst
> index 47323494d119..7cd04d3261ad 100644
> --- a/Documentation/admin-guide/LSM/ipe.rst
> +++ b/Documentation/admin-guide/LSM/ipe.rst
> @@ -269,7 +269,7 @@ in the kernel. This file is write-only and accepts a PKCS#7 signed
>   policy. Two checks will always be performed on this policy: First, the
>   ``policy_names`` must match with the updated version and the existing
>   version. Second the updated policy must have a policy version greater than
> -or equal to the currently-running version. This is to prevent rollback attacks.
> +to the currently-running version. This is to prevent rollback attacks.

The "to" here should also be removed.

-Fan
>   
>   The ``delete`` file is used to remove a policy that is no longer needed.
>   This file is write-only and accepts a value of ``1`` to delete the policy.
> diff --git a/security/ipe/policy.c b/security/ipe/policy.c
> index 3a0069c6d5af..45f7d6a0ed23 100644
> --- a/security/ipe/policy.c
> +++ b/security/ipe/policy.c
> @@ -106,7 +106,7 @@ int ipe_update_policy(struct inode *root, const char *text, size_t textlen,
>   		goto err;
>   	}
>   
> -	if (ver_to_u64(old) > ver_to_u64(new)) {
> +	if (ver_to_u64(old) >= ver_to_u64(new)) {
>   		rc = -ESTALE;
>   		goto err;
>   	}

Re: [PATCH v2 1/2] ipe: return -ESTALE instead of -EINVAL on update when new policy has a lower version

[Fan Wu]

On 9/25/2024 1:42 PM, luca.boccassi@gmail.com wrote:
> From: Luca Boccassi <bluca@debian.org>
> 
> When loading policies in userspace we want a recognizable error when an
> update attempts to use an old policy, as that is an error that needs
> to be treated differently from an invalid policy. Use -ESTALE as it is
> clear enough for an update mechanism.
> 
> Signed-off-by: Luca Boccassi <bluca@debian.org>
> Reviewed-by: Serge Hallyn <serge@hallyn.com>
> ---
>   security/ipe/policy.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/security/ipe/policy.c b/security/ipe/policy.c
> index bf5aa97911e1..3a0069c6d5af 100644
> --- a/security/ipe/policy.c
> +++ b/security/ipe/policy.c
> @@ -107,7 +107,7 @@ int ipe_update_policy(struct inode *root, const char *text, size_t textlen,
>   	}
>   
>   	if (ver_to_u64(old) > ver_to_u64(new)) {
> -		rc = -EINVAL;
> +		rc = -ESTALE;
>   		goto err;
>   	}
>

Acked-by: Fan Wu <wufan@linux.microsoft.com>

I'm still in the process of applying kernel.org account, once I get 
everything setup I will include this in my tree.

If that process take too long time I may ask Paul to merge this from the 
lsm tree in next merge window.

-Fan