| Message ID | 20231024213525.361332-5-paul@paul-moore.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | LSM syscall tweaks | expand |
On 10/24/2023 2:35 PM, Paul Moore wrote: > Zero out all of the size counters in the -E2BIG case (buffer too > small) to help make the current code a bit more robust in the face of > future code changes. I don't see how this change would have the described effect. What it looks like it would do is change the return from -E2BIG to 0, which would not have the desired result. > > Signed-off-by: Paul Moore <paul@paul-moore.com> > --- > security/security.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/security/security.c b/security/security.c > index 988483fcf153..9c63acded4ee 100644 > --- a/security/security.c > +++ b/security/security.c > @@ -3951,8 +3951,9 @@ int security_getselfattr(unsigned int attr, struct lsm_ctx __user *uctx, > continue; > } > if (rc == -E2BIG) { > - toobig = true; > + rc = 0; > left = 0; > + toobig = true; > } else if (rc < 0) > return rc; > else
On Tue, Oct 24, 2023 at 6:23 PM Casey Schaufler <casey@schaufler-ca.com> wrote: > On 10/24/2023 2:35 PM, Paul Moore wrote: > > Zero out all of the size counters in the -E2BIG case (buffer too > > small) to help make the current code a bit more robust in the face of > > future code changes. > > I don't see how this change would have the described effect. > What it looks like it would do is change the return from -E2BIG > to 0, which would not have the desired result. When @toobig is true, which it will be when one of the individual LSMs return -E2BIG, the return value of security_getselfattr() is fixed to -E2BIG (check the if-statements at the end of the function). Setting @rc to zero as in this patch simply preserves some sanity in the @count variable as we are no longer subtracting the E2BIG errno from the @count value. Granted, in the @toobig case, @count doesn't do anything meaningful, but I believe this does harden the code against future changes. Look at the discussion between Mickaël and I in the v15 04/11 patch for more background. https://lore.kernel.org/linux-security-module/20230912205658.3432-5-casey@schaufler-ca.com > > Signed-off-by: Paul Moore <paul@paul-moore.com> > > --- > > security/security.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/security/security.c b/security/security.c > > index 988483fcf153..9c63acded4ee 100644 > > --- a/security/security.c > > +++ b/security/security.c > > @@ -3951,8 +3951,9 @@ int security_getselfattr(unsigned int attr, struct lsm_ctx __user *uctx, > > continue; > > } > > if (rc == -E2BIG) { > > - toobig = true; > > + rc = 0; > > left = 0; > > + toobig = true; > > } else if (rc < 0) > > return rc; > > else
On 10/24/2023 6:43 PM, Paul Moore wrote: > On Tue, Oct 24, 2023 at 6:23 PM Casey Schaufler <casey@schaufler-ca.com> wrote: >> On 10/24/2023 2:35 PM, Paul Moore wrote: >>> Zero out all of the size counters in the -E2BIG case (buffer too >>> small) to help make the current code a bit more robust in the face of >>> future code changes. >> I don't see how this change would have the described effect. >> What it looks like it would do is change the return from -E2BIG >> to 0, which would not have the desired result. > When @toobig is true, which it will be when one of the individual LSMs > return -E2BIG, the return value of security_getselfattr() is fixed to > -E2BIG (check the if-statements at the end of the function). Setting > @rc to zero as in this patch simply preserves some sanity in the > @count variable as we are no longer subtracting the E2BIG errno from > the @count value. Granted, in the @toobig case, @count doesn't do > anything meaningful, but I believe this does harden the code against > future changes. > > Look at the discussion between Mickaël and I in the v15 04/11 patch > for more background. > > https://lore.kernel.org/linux-security-module/20230912205658.3432-5-casey@schaufler-ca.com OK. My bad for not looking beyond the patch. Acked-by: Casey Schaufler <casey@schaufler-ca.com> > >>> Signed-off-by: Paul Moore <paul@paul-moore.com> >>> --- >>> security/security.c | 3 ++- >>> 1 file changed, 2 insertions(+), 1 deletion(-) >>> >>> diff --git a/security/security.c b/security/security.c >>> index 988483fcf153..9c63acded4ee 100644 >>> --- a/security/security.c >>> +++ b/security/security.c >>> @@ -3951,8 +3951,9 @@ int security_getselfattr(unsigned int attr, struct lsm_ctx __user *uctx, >>> continue; >>> } >>> if (rc == -E2BIG) { >>> - toobig = true; >>> + rc = 0; >>> left = 0; >>> + toobig = true; >>> } else if (rc < 0) >>> return rc; >>> else
On Wed, Oct 25, 2023 at 11:19 AM Casey Schaufler <casey@schaufler-ca.com> wrote: > On 10/24/2023 6:43 PM, Paul Moore wrote: > > On Tue, Oct 24, 2023 at 6:23 PM Casey Schaufler <casey@schaufler-ca.com> wrote: > >> On 10/24/2023 2:35 PM, Paul Moore wrote: > >>> Zero out all of the size counters in the -E2BIG case (buffer too > >>> small) to help make the current code a bit more robust in the face of > >>> future code changes. > >> I don't see how this change would have the described effect. > >> What it looks like it would do is change the return from -E2BIG > >> to 0, which would not have the desired result. > > When @toobig is true, which it will be when one of the individual LSMs > > return -E2BIG, the return value of security_getselfattr() is fixed to > > -E2BIG (check the if-statements at the end of the function). Setting > > @rc to zero as in this patch simply preserves some sanity in the > > @count variable as we are no longer subtracting the E2BIG errno from > > the @count value. Granted, in the @toobig case, @count doesn't do > > anything meaningful, but I believe this does harden the code against > > future changes. > > > > Look at the discussion between Mickaël and I in the v15 04/11 patch > > for more background. > > > > https://lore.kernel.org/linux-security-module/20230912205658.3432-5-casey@schaufler-ca.com > > OK. My bad for not looking beyond the patch. No problem, we all get caught out from time to time, thanks for the review/ACK.
On Tue, Oct 24, 2023 at 05:35:27PM -0400, Paul Moore wrote: > Zero out all of the size counters in the -E2BIG case (buffer too > small) to help make the current code a bit more robust in the face of > future code changes. > > Signed-off-by: Paul Moore <paul@paul-moore.com> Reviewed-by: Mickaël Salaün <mic@digikod.net> > --- > security/security.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/security/security.c b/security/security.c > index 988483fcf153..9c63acded4ee 100644 > --- a/security/security.c > +++ b/security/security.c > @@ -3951,8 +3951,9 @@ int security_getselfattr(unsigned int attr, struct lsm_ctx __user *uctx, > continue; > } > if (rc == -E2BIG) { > - toobig = true; > + rc = 0; > left = 0; > + toobig = true; > } else if (rc < 0) > return rc; > else > -- > 2.42.0 >
diff --git a/security/security.c b/security/security.c index 988483fcf153..9c63acded4ee 100644 --- a/security/security.c +++ b/security/security.c @@ -3951,8 +3951,9 @@ int security_getselfattr(unsigned int attr, struct lsm_ctx __user *uctx, continue; } if (rc == -E2BIG) { - toobig = true; + rc = 0; left = 0; + toobig = true; } else if (rc < 0) return rc; else
Zero out all of the size counters in the -E2BIG case (buffer too small) to help make the current code a bit more robust in the face of future code changes. Signed-off-by: Paul Moore <paul@paul-moore.com> --- security/security.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)