| Message ID | 20240314022202.599471-2-paul@paul-moore.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | lsm: handle the NULL buffer case in lsm_fill_user_ctx() | expand |
On Wed, Mar 13, 2024 at 10:22 PM Paul Moore <paul@paul-moore.com> wrote: > > Passing a NULL buffer into the lsm_get_self_attr() syscall is a valid > way to quickly determine the minimum size of the buffer needed to for > the syscall to return all of the LSM attributes to the caller. > Unfortunately we/I broke that behavior in commit d7cf3412a9f6 > ("lsm: consolidate buffer size handling into lsm_fill_user_ctx()") > such that it returned an error to the caller; this patch restores the > original desired behavior of using the NULL buffer as a quick way to > correctly size the attribute buffer. > > Cc: stable@vger.kernel.org > Fixes: d7cf3412a9f6 ("lsm: consolidate buffer size handling into lsm_fill_user_ctx()") > Signed-off-by: Paul Moore <paul@paul-moore.com> > --- > security/security.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) Merged into lsm/stable-6.9, if anyone has any objections please make them know soon. > diff --git a/security/security.c b/security/security.c > index 5b2e0a15377d..7e118858b545 100644 > --- a/security/security.c > +++ b/security/security.c > @@ -780,7 +780,9 @@ static int lsm_superblock_alloc(struct super_block *sb) > * @id: LSM id > * @flags: LSM defined flags > * > - * Fill all of the fields in a userspace lsm_ctx structure. > + * Fill all of the fields in a userspace lsm_ctx structure. If @uctx is NULL > + * simply calculate the required size to output via @utc_len and return > + * success. > * > * Returns 0 on success, -E2BIG if userspace buffer is not large enough, > * -EFAULT on a copyout error, -ENOMEM if memory can't be allocated. > @@ -799,6 +801,10 @@ int lsm_fill_user_ctx(struct lsm_ctx __user *uctx, u32 *uctx_len, > goto out; > } > > + /* no buffer - return success/0 and set @uctx_len to the req size */ > + if (!uctx) > + goto out; > + > nctx = kzalloc(nctx_len, GFP_KERNEL); > if (nctx == NULL) { > rc = -ENOMEM; > -- > 2.44.0
On Wed, Mar 13, 2024 at 10:22:03PM -0400, Paul Moore wrote: > Passing a NULL buffer into the lsm_get_self_attr() syscall is a valid > way to quickly determine the minimum size of the buffer needed to for > the syscall to return all of the LSM attributes to the caller. > Unfortunately we/I broke that behavior in commit d7cf3412a9f6 > ("lsm: consolidate buffer size handling into lsm_fill_user_ctx()") > such that it returned an error to the caller; this patch restores the > original desired behavior of using the NULL buffer as a quick way to > correctly size the attribute buffer. > > Cc: stable@vger.kernel.org > Fixes: d7cf3412a9f6 ("lsm: consolidate buffer size handling into lsm_fill_user_ctx()") > Signed-off-by: Paul Moore <paul@paul-moore.com> > --- > security/security.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/security/security.c b/security/security.c > index 5b2e0a15377d..7e118858b545 100644 > --- a/security/security.c > +++ b/security/security.c > @@ -780,7 +780,9 @@ static int lsm_superblock_alloc(struct super_block *sb) > * @id: LSM id > * @flags: LSM defined flags > * > - * Fill all of the fields in a userspace lsm_ctx structure. > + * Fill all of the fields in a userspace lsm_ctx structure. If @uctx is NULL > + * simply calculate the required size to output via @utc_len and return > + * success. > * > * Returns 0 on success, -E2BIG if userspace buffer is not large enough, > * -EFAULT on a copyout error, -ENOMEM if memory can't be allocated. > @@ -799,6 +801,10 @@ int lsm_fill_user_ctx(struct lsm_ctx __user *uctx, u32 *uctx_len, > goto out; > } > > + /* no buffer - return success/0 and set @uctx_len to the req size */ > + if (!uctx) > + goto out; If the user just passes in *uctx_len=0, then they will get -E2BIG but still will get the length in *uctx_len. To use it this new way, they have to first set *uctx_len to a value larger than nctx_len could possibly be, else they'll... still get -E2BIG. So I'm not sure this patch has value. > nctx = kzalloc(nctx_len, GFP_KERNEL); > if (nctx == NULL) { > rc = -ENOMEM; > -- > 2.44.0 >
On Fri, Mar 15, 2024 at 11:02 AM Serge E. Hallyn <serge@hallyn.com> wrote: > On Wed, Mar 13, 2024 at 10:22:03PM -0400, Paul Moore wrote: > > Passing a NULL buffer into the lsm_get_self_attr() syscall is a valid > > way to quickly determine the minimum size of the buffer needed to for > > the syscall to return all of the LSM attributes to the caller. > > Unfortunately we/I broke that behavior in commit d7cf3412a9f6 > > ("lsm: consolidate buffer size handling into lsm_fill_user_ctx()") > > such that it returned an error to the caller; this patch restores the > > original desired behavior of using the NULL buffer as a quick way to > > correctly size the attribute buffer. > > > > Cc: stable@vger.kernel.org > > Fixes: d7cf3412a9f6 ("lsm: consolidate buffer size handling into lsm_fill_user_ctx()") > > Signed-off-by: Paul Moore <paul@paul-moore.com> > > --- > > security/security.c | 8 +++++++- > > 1 file changed, 7 insertions(+), 1 deletion(-) > > > > diff --git a/security/security.c b/security/security.c > > index 5b2e0a15377d..7e118858b545 100644 > > --- a/security/security.c > > +++ b/security/security.c > > @@ -780,7 +780,9 @@ static int lsm_superblock_alloc(struct super_block *sb) > > * @id: LSM id > > * @flags: LSM defined flags > > * > > - * Fill all of the fields in a userspace lsm_ctx structure. > > + * Fill all of the fields in a userspace lsm_ctx structure. If @uctx is NULL > > + * simply calculate the required size to output via @utc_len and return > > + * success. > > * > > * Returns 0 on success, -E2BIG if userspace buffer is not large enough, > > * -EFAULT on a copyout error, -ENOMEM if memory can't be allocated. > > @@ -799,6 +801,10 @@ int lsm_fill_user_ctx(struct lsm_ctx __user *uctx, u32 *uctx_len, > > goto out; > > } > > > > + /* no buffer - return success/0 and set @uctx_len to the req size */ > > + if (!uctx) > > + goto out; > > If the user just passes in *uctx_len=0, then they will get -E2BIG > but still will get the length in *uctx_len. Right, which is the desired behavior, this patch allows userspace to not have to worry about supplying a buffer if they just want to check the required size, regardless of the value passed in @uctx_len. > To use it this new way, they have to first set *uctx_len to a > value larger than nctx_len could possibly be, else they'll... > still get -E2BIG. True. > So I'm not sure this patch has value. Oh, but it resolves a kselftest failure in a released kernel, that's worth a *lot* in my mind. -- paul-moore.com
On Fri, Mar 15, 2024 at 11:18:38AM -0400, Paul Moore wrote: > On Fri, Mar 15, 2024 at 11:02 AM Serge E. Hallyn <serge@hallyn.com> wrote: > > On Wed, Mar 13, 2024 at 10:22:03PM -0400, Paul Moore wrote: > > > Passing a NULL buffer into the lsm_get_self_attr() syscall is a valid > > > way to quickly determine the minimum size of the buffer needed to for > > > the syscall to return all of the LSM attributes to the caller. > > > Unfortunately we/I broke that behavior in commit d7cf3412a9f6 > > > ("lsm: consolidate buffer size handling into lsm_fill_user_ctx()") > > > such that it returned an error to the caller; this patch restores the > > > original desired behavior of using the NULL buffer as a quick way to > > > correctly size the attribute buffer. > > > > > > Cc: stable@vger.kernel.org > > > Fixes: d7cf3412a9f6 ("lsm: consolidate buffer size handling into lsm_fill_user_ctx()") > > > Signed-off-by: Paul Moore <paul@paul-moore.com> > > > --- > > > security/security.c | 8 +++++++- > > > 1 file changed, 7 insertions(+), 1 deletion(-) > > > > > > diff --git a/security/security.c b/security/security.c > > > index 5b2e0a15377d..7e118858b545 100644 > > > --- a/security/security.c > > > +++ b/security/security.c > > > @@ -780,7 +780,9 @@ static int lsm_superblock_alloc(struct super_block *sb) > > > * @id: LSM id > > > * @flags: LSM defined flags > > > * > > > - * Fill all of the fields in a userspace lsm_ctx structure. > > > + * Fill all of the fields in a userspace lsm_ctx structure. If @uctx is NULL > > > + * simply calculate the required size to output via @utc_len and return > > > + * success. > > > * > > > * Returns 0 on success, -E2BIG if userspace buffer is not large enough, > > > * -EFAULT on a copyout error, -ENOMEM if memory can't be allocated. > > > @@ -799,6 +801,10 @@ int lsm_fill_user_ctx(struct lsm_ctx __user *uctx, u32 *uctx_len, > > > goto out; > > > } > > > > > > + /* no buffer - return success/0 and set @uctx_len to the req size */ > > > + if (!uctx) > > > + goto out; > > > > If the user just passes in *uctx_len=0, then they will get -E2BIG > > but still will get the length in *uctx_len. > > Right, which is the desired behavior, this patch allows userspace to > not have to worry about supplying a buffer if they just want to check > the required size, regardless of the value passed in @uctx_len. To be clear - I was saying that is the behavior before your patch. (If I'm not wrong) This patch changes behavior as follows: Before this patch, there was one way for the user to query the required size: send in a length of 0, get back E2BIG and check the *uctx_len you had passed in for the required value. After this patch, there are two ways - one resulting in -E2BIG as before, and one requiring uctx_len to be set to a large value *and* uctx to be NULL, getting rc=0 and taking that to mean look at the new uctx_len value. > > To use it this new way, they have to first set *uctx_len to a > > value larger than nctx_len could possibly be, else they'll... > > still get -E2BIG. > > True. > > > So I'm not sure this patch has value. > > Oh, but it resolves a kselftest failure in a released kernel, that's > worth a *lot* in my mind. Sorry, you didn't mention the test in the commit message. Which testcase? -serge
On Fri, Mar 15, 2024 at 11:55 AM Serge E. Hallyn <serge@hallyn.com> wrote: > On Fri, Mar 15, 2024 at 11:18:38AM -0400, Paul Moore wrote: > > On Fri, Mar 15, 2024 at 11:02 AM Serge E. Hallyn <serge@hallyn.com> wrote: > > > On Wed, Mar 13, 2024 at 10:22:03PM -0400, Paul Moore wrote: > > > > Passing a NULL buffer into the lsm_get_self_attr() syscall is a valid > > > > way to quickly determine the minimum size of the buffer needed to for > > > > the syscall to return all of the LSM attributes to the caller. > > > > Unfortunately we/I broke that behavior in commit d7cf3412a9f6 > > > > ("lsm: consolidate buffer size handling into lsm_fill_user_ctx()") > > > > such that it returned an error to the caller; this patch restores the > > > > original desired behavior of using the NULL buffer as a quick way to > > > > correctly size the attribute buffer. > > > > > > > > Cc: stable@vger.kernel.org > > > > Fixes: d7cf3412a9f6 ("lsm: consolidate buffer size handling into lsm_fill_user_ctx()") > > > > Signed-off-by: Paul Moore <paul@paul-moore.com> > > > > --- > > > > security/security.c | 8 +++++++- > > > > 1 file changed, 7 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/security/security.c b/security/security.c > > > > index 5b2e0a15377d..7e118858b545 100644 > > > > --- a/security/security.c > > > > +++ b/security/security.c > > > > @@ -780,7 +780,9 @@ static int lsm_superblock_alloc(struct super_block *sb) > > > > * @id: LSM id > > > > * @flags: LSM defined flags > > > > * > > > > - * Fill all of the fields in a userspace lsm_ctx structure. > > > > + * Fill all of the fields in a userspace lsm_ctx structure. If @uctx is NULL > > > > + * simply calculate the required size to output via @utc_len and return > > > > + * success. > > > > * > > > > * Returns 0 on success, -E2BIG if userspace buffer is not large enough, > > > > * -EFAULT on a copyout error, -ENOMEM if memory can't be allocated. > > > > @@ -799,6 +801,10 @@ int lsm_fill_user_ctx(struct lsm_ctx __user *uctx, u32 *uctx_len, > > > > goto out; > > > > } > > > > > > > > + /* no buffer - return success/0 and set @uctx_len to the req size */ > > > > + if (!uctx) > > > > + goto out; > > > > > > If the user just passes in *uctx_len=0, then they will get -E2BIG > > > but still will get the length in *uctx_len. > > > > Right, which is the desired behavior, this patch allows userspace to > > not have to worry about supplying a buffer if they just want to check > > the required size, regardless of the value passed in @uctx_len. > > To be clear - I was saying that is the behavior before your patch. > > (If I'm not wrong) > > This patch changes behavior as follows: > > Before this patch, there was one way for the user to query the required > size: send in a length of 0, get back E2BIG and check the *uctx_len you > had passed in for the required value. > > After this patch, there are two ways - one resulting in -E2BIG as before, > and one requiring uctx_len to be set to a large value *and* uctx to be > NULL, getting rc=0 and taking that to mean look at the new uctx_len value. Yes, agreed, no notes. > > > To use it this new way, they have to first set *uctx_len to a > > > value larger than nctx_len could possibly be, else they'll... > > > still get -E2BIG. > > > > True. > > > > > So I'm not sure this patch has value. > > > > Oh, but it resolves a kselftest failure in a released kernel, that's > > worth a *lot* in my mind. > > Sorry, you didn't mention the test in the commit message. Which testcase? See the tools/testing/selftests/lsm/lsm_get_self_attr_test.c tests.
On 3/15/2024 8:02 AM, Serge E. Hallyn wrote: > On Wed, Mar 13, 2024 at 10:22:03PM -0400, Paul Moore wrote: >> Passing a NULL buffer into the lsm_get_self_attr() syscall is a valid >> way to quickly determine the minimum size of the buffer needed to for >> the syscall to return all of the LSM attributes to the caller. >> Unfortunately we/I broke that behavior in commit d7cf3412a9f6 >> ("lsm: consolidate buffer size handling into lsm_fill_user_ctx()") >> such that it returned an error to the caller; this patch restores the >> original desired behavior of using the NULL buffer as a quick way to >> correctly size the attribute buffer. >> >> Cc: stable@vger.kernel.org >> Fixes: d7cf3412a9f6 ("lsm: consolidate buffer size handling into lsm_fill_user_ctx()") >> Signed-off-by: Paul Moore <paul@paul-moore.com> >> --- >> security/security.c | 8 +++++++- >> 1 file changed, 7 insertions(+), 1 deletion(-) >> >> diff --git a/security/security.c b/security/security.c >> index 5b2e0a15377d..7e118858b545 100644 >> --- a/security/security.c >> +++ b/security/security.c >> @@ -780,7 +780,9 @@ static int lsm_superblock_alloc(struct super_block *sb) >> * @id: LSM id >> * @flags: LSM defined flags >> * >> - * Fill all of the fields in a userspace lsm_ctx structure. >> + * Fill all of the fields in a userspace lsm_ctx structure. If @uctx is NULL >> + * simply calculate the required size to output via @utc_len and return >> + * success. >> * >> * Returns 0 on success, -E2BIG if userspace buffer is not large enough, >> * -EFAULT on a copyout error, -ENOMEM if memory can't be allocated. >> @@ -799,6 +801,10 @@ int lsm_fill_user_ctx(struct lsm_ctx __user *uctx, u32 *uctx_len, >> goto out; >> } >> >> + /* no buffer - return success/0 and set @uctx_len to the req size */ >> + if (!uctx) >> + goto out; > If the user just passes in *uctx_len=0, then they will get -E2BIG > but still will get the length in *uctx_len. Yes. > To use it this new way, they have to first set *uctx_len to a > value larger than nctx_len could possibly be, else they'll... > still get -E2BIG. Not sure I understand the problem. A return of 0 or E2BIG gets the caller the size. > So I'm not sure this patch has value. > >> nctx = kzalloc(nctx_len, GFP_KERNEL); >> if (nctx == NULL) { >> rc = -ENOMEM; >> -- >> 2.44.0 >>
On Fri, Mar 15, 2024 at 09:08:47AM -0700, Casey Schaufler wrote: > On 3/15/2024 8:02 AM, Serge E. Hallyn wrote: > > On Wed, Mar 13, 2024 at 10:22:03PM -0400, Paul Moore wrote: > >> Passing a NULL buffer into the lsm_get_self_attr() syscall is a valid > >> way to quickly determine the minimum size of the buffer needed to for > >> the syscall to return all of the LSM attributes to the caller. > >> Unfortunately we/I broke that behavior in commit d7cf3412a9f6 > >> ("lsm: consolidate buffer size handling into lsm_fill_user_ctx()") > >> such that it returned an error to the caller; this patch restores the > >> original desired behavior of using the NULL buffer as a quick way to > >> correctly size the attribute buffer. > >> > >> Cc: stable@vger.kernel.org > >> Fixes: d7cf3412a9f6 ("lsm: consolidate buffer size handling into lsm_fill_user_ctx()") > >> Signed-off-by: Paul Moore <paul@paul-moore.com> > >> --- > >> security/security.c | 8 +++++++- > >> 1 file changed, 7 insertions(+), 1 deletion(-) > >> > >> diff --git a/security/security.c b/security/security.c > >> index 5b2e0a15377d..7e118858b545 100644 > >> --- a/security/security.c > >> +++ b/security/security.c > >> @@ -780,7 +780,9 @@ static int lsm_superblock_alloc(struct super_block *sb) > >> * @id: LSM id > >> * @flags: LSM defined flags > >> * > >> - * Fill all of the fields in a userspace lsm_ctx structure. > >> + * Fill all of the fields in a userspace lsm_ctx structure. If @uctx is NULL > >> + * simply calculate the required size to output via @utc_len and return > >> + * success. > >> * > >> * Returns 0 on success, -E2BIG if userspace buffer is not large enough, > >> * -EFAULT on a copyout error, -ENOMEM if memory can't be allocated. > >> @@ -799,6 +801,10 @@ int lsm_fill_user_ctx(struct lsm_ctx __user *uctx, u32 *uctx_len, > >> goto out; > >> } > >> > >> + /* no buffer - return success/0 and set @uctx_len to the req size */ > >> + if (!uctx) > >> + goto out; > > If the user just passes in *uctx_len=0, then they will get -E2BIG > > but still will get the length in *uctx_len. > > Yes. > > > To use it this new way, they have to first set *uctx_len to a > > value larger than nctx_len could possibly be, else they'll... > > still get -E2BIG. > > Not sure I understand the problem. A return of 0 or E2BIG gets the > caller the size. The problem is that there are two ways of doing the same thing, with different behavior. People are bound to get it wrong at some point, and it's more corner cases to try and maintain (once we start).
On 3/15/2024 8:55 AM, Serge E. Hallyn wrote: > On Fri, Mar 15, 2024 at 11:18:38AM -0400, Paul Moore wrote: >> On Fri, Mar 15, 2024 at 11:02 AM Serge E. Hallyn <serge@hallyn.com> wrote: >>> On Wed, Mar 13, 2024 at 10:22:03PM -0400, Paul Moore wrote: >>>> Passing a NULL buffer into the lsm_get_self_attr() syscall is a valid >>>> way to quickly determine the minimum size of the buffer needed to for >>>> the syscall to return all of the LSM attributes to the caller. >>>> Unfortunately we/I broke that behavior in commit d7cf3412a9f6 >>>> ("lsm: consolidate buffer size handling into lsm_fill_user_ctx()") >>>> such that it returned an error to the caller; this patch restores the >>>> original desired behavior of using the NULL buffer as a quick way to >>>> correctly size the attribute buffer. >>>> >>>> Cc: stable@vger.kernel.org >>>> Fixes: d7cf3412a9f6 ("lsm: consolidate buffer size handling into lsm_fill_user_ctx()") >>>> Signed-off-by: Paul Moore <paul@paul-moore.com> >>>> --- >>>> security/security.c | 8 +++++++- >>>> 1 file changed, 7 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/security/security.c b/security/security.c >>>> index 5b2e0a15377d..7e118858b545 100644 >>>> --- a/security/security.c >>>> +++ b/security/security.c >>>> @@ -780,7 +780,9 @@ static int lsm_superblock_alloc(struct super_block *sb) >>>> * @id: LSM id >>>> * @flags: LSM defined flags >>>> * >>>> - * Fill all of the fields in a userspace lsm_ctx structure. >>>> + * Fill all of the fields in a userspace lsm_ctx structure. If @uctx is NULL >>>> + * simply calculate the required size to output via @utc_len and return >>>> + * success. >>>> * >>>> * Returns 0 on success, -E2BIG if userspace buffer is not large enough, >>>> * -EFAULT on a copyout error, -ENOMEM if memory can't be allocated. >>>> @@ -799,6 +801,10 @@ int lsm_fill_user_ctx(struct lsm_ctx __user *uctx, u32 *uctx_len, >>>> goto out; >>>> } >>>> >>>> + /* no buffer - return success/0 and set @uctx_len to the req size */ >>>> + if (!uctx) >>>> + goto out; >>> If the user just passes in *uctx_len=0, then they will get -E2BIG >>> but still will get the length in *uctx_len. >> Right, which is the desired behavior, this patch allows userspace to >> not have to worry about supplying a buffer if they just want to check >> the required size, regardless of the value passed in @uctx_len. > To be clear - I was saying that is the behavior before your patch. > > (If I'm not wrong) > > This patch changes behavior as follows: > > Before this patch, there was one way for the user to query the required > size: send in a length of 0, get back E2BIG and check the *uctx_len you > had passed in for the required value. > > After this patch, there are two ways - one resulting in -E2BIG as before, > and one requiring uctx_len to be set to a large value *and* uctx to be > NULL, getting rc=0 and taking that to mean look at the new uctx_len value. > >>> To use it this new way, they have to first set *uctx_len to a >>> value larger than nctx_len could possibly be, else they'll... >>> still get -E2BIG. >> True. >> >>> So I'm not sure this patch has value. >> Oh, but it resolves a kselftest failure in a released kernel, that's >> worth a *lot* in my mind. > Sorry, you didn't mention the test in the commit message. Which testcase? tools/testing/selftests/lsm/lsm_get_self_attr_test.c > > -serge >
On Fri, Mar 15, 2024 at 12:13 PM Serge E. Hallyn <serge@hallyn.com> wrote: > On Fri, Mar 15, 2024 at 09:08:47AM -0700, Casey Schaufler wrote: > > On 3/15/2024 8:02 AM, Serge E. Hallyn wrote: > > > On Wed, Mar 13, 2024 at 10:22:03PM -0400, Paul Moore wrote: > > >> Passing a NULL buffer into the lsm_get_self_attr() syscall is a valid > > >> way to quickly determine the minimum size of the buffer needed to for > > >> the syscall to return all of the LSM attributes to the caller. > > >> Unfortunately we/I broke that behavior in commit d7cf3412a9f6 > > >> ("lsm: consolidate buffer size handling into lsm_fill_user_ctx()") > > >> such that it returned an error to the caller; this patch restores the > > >> original desired behavior of using the NULL buffer as a quick way to > > >> correctly size the attribute buffer. > > >> > > >> Cc: stable@vger.kernel.org > > >> Fixes: d7cf3412a9f6 ("lsm: consolidate buffer size handling into lsm_fill_user_ctx()") > > >> Signed-off-by: Paul Moore <paul@paul-moore.com> > > >> --- > > >> security/security.c | 8 +++++++- > > >> 1 file changed, 7 insertions(+), 1 deletion(-) > > >> > > >> diff --git a/security/security.c b/security/security.c > > >> index 5b2e0a15377d..7e118858b545 100644 > > >> --- a/security/security.c > > >> +++ b/security/security.c > > >> @@ -780,7 +780,9 @@ static int lsm_superblock_alloc(struct super_block *sb) > > >> * @id: LSM id > > >> * @flags: LSM defined flags > > >> * > > >> - * Fill all of the fields in a userspace lsm_ctx structure. > > >> + * Fill all of the fields in a userspace lsm_ctx structure. If @uctx is NULL > > >> + * simply calculate the required size to output via @utc_len and return > > >> + * success. > > >> * > > >> * Returns 0 on success, -E2BIG if userspace buffer is not large enough, > > >> * -EFAULT on a copyout error, -ENOMEM if memory can't be allocated. > > >> @@ -799,6 +801,10 @@ int lsm_fill_user_ctx(struct lsm_ctx __user *uctx, u32 *uctx_len, > > >> goto out; > > >> } > > >> > > >> + /* no buffer - return success/0 and set @uctx_len to the req size */ > > >> + if (!uctx) > > >> + goto out; > > > If the user just passes in *uctx_len=0, then they will get -E2BIG > > > but still will get the length in *uctx_len. > > > > Yes. > > > > > To use it this new way, they have to first set *uctx_len to a > > > value larger than nctx_len could possibly be, else they'll... > > > still get -E2BIG. > > > > Not sure I understand the problem. A return of 0 or E2BIG gets the > > caller the size. > > The problem is that there are two ways of doing the same thing, with > different behavior. People are bound to get it wrong at some point, > and it's more corner cases to try and maintain (once we start). I have a different perspective on this, you can supply either a NULL buffer and/or a buffer that is too small, including a size of zero, and you'll get back an -E2BIG and a minimum buffer size. What's the old wisdom, be conservative in what you send and liberal in what you accept?
On Fri, Mar 15, 2024 at 12:19:05PM -0400, Paul Moore wrote: > On Fri, Mar 15, 2024 at 12:13 PM Serge E. Hallyn <serge@hallyn.com> wrote: > > On Fri, Mar 15, 2024 at 09:08:47AM -0700, Casey Schaufler wrote: > > > On 3/15/2024 8:02 AM, Serge E. Hallyn wrote: > > > > On Wed, Mar 13, 2024 at 10:22:03PM -0400, Paul Moore wrote: > > > >> Passing a NULL buffer into the lsm_get_self_attr() syscall is a valid > > > >> way to quickly determine the minimum size of the buffer needed to for > > > >> the syscall to return all of the LSM attributes to the caller. > > > >> Unfortunately we/I broke that behavior in commit d7cf3412a9f6 > > > >> ("lsm: consolidate buffer size handling into lsm_fill_user_ctx()") > > > >> such that it returned an error to the caller; this patch restores the > > > >> original desired behavior of using the NULL buffer as a quick way to > > > >> correctly size the attribute buffer. > > > >> > > > >> Cc: stable@vger.kernel.org > > > >> Fixes: d7cf3412a9f6 ("lsm: consolidate buffer size handling into lsm_fill_user_ctx()") > > > >> Signed-off-by: Paul Moore <paul@paul-moore.com> > > > >> --- > > > >> security/security.c | 8 +++++++- > > > >> 1 file changed, 7 insertions(+), 1 deletion(-) > > > >> > > > >> diff --git a/security/security.c b/security/security.c > > > >> index 5b2e0a15377d..7e118858b545 100644 > > > >> --- a/security/security.c > > > >> +++ b/security/security.c > > > >> @@ -780,7 +780,9 @@ static int lsm_superblock_alloc(struct super_block *sb) > > > >> * @id: LSM id > > > >> * @flags: LSM defined flags > > > >> * > > > >> - * Fill all of the fields in a userspace lsm_ctx structure. > > > >> + * Fill all of the fields in a userspace lsm_ctx structure. If @uctx is NULL > > > >> + * simply calculate the required size to output via @utc_len and return > > > >> + * success. > > > >> * > > > >> * Returns 0 on success, -E2BIG if userspace buffer is not large enough, > > > >> * -EFAULT on a copyout error, -ENOMEM if memory can't be allocated. > > > >> @@ -799,6 +801,10 @@ int lsm_fill_user_ctx(struct lsm_ctx __user *uctx, u32 *uctx_len, > > > >> goto out; > > > >> } > > > >> > > > >> + /* no buffer - return success/0 and set @uctx_len to the req size */ > > > >> + if (!uctx) > > > >> + goto out; > > > > If the user just passes in *uctx_len=0, then they will get -E2BIG > > > > but still will get the length in *uctx_len. > > > > > > Yes. > > > > > > > To use it this new way, they have to first set *uctx_len to a > > > > value larger than nctx_len could possibly be, else they'll... > > > > still get -E2BIG. > > > > > > Not sure I understand the problem. A return of 0 or E2BIG gets the > > > caller the size. > > > > The problem is that there are two ways of doing the same thing, with > > different behavior. People are bound to get it wrong at some point, > > and it's more corner cases to try and maintain (once we start). > > I have a different perspective on this, you can supply either a NULL > buffer and/or a buffer that is too small, including a size of zero, > and you'll get back an -E2BIG and a minimum buffer size. What's the > old wisdom, be conservative in what you send and liberal in what you > accept? But if you pass a NULL uctx, then surely you should send *uctx_len=0. And that is already handled. What you are adding is that the user can pass NULL uctx, but a large uctx_len value. Perhaps should change my objection, and say that I would prefer the comment fix to suggest passing in uctx_len=0 and uctx=NULL, and expect a -E2BIG. The NULL check can stay (though I still think it should return -E2BIG). Because with the current comment update, the user may pass in uctx=NULL, but the actual rv will change between 0 and -E2BIG depending on the uctx_len they sent in. Which is whack, since the incoming value means nothing. -serge
On Fri, Mar 15, 2024 at 12:28 PM Serge E. Hallyn <serge@hallyn.com> wrote: > On Fri, Mar 15, 2024 at 12:19:05PM -0400, Paul Moore wrote: > > On Fri, Mar 15, 2024 at 12:13 PM Serge E. Hallyn <serge@hallyn.com> wrote: > > > On Fri, Mar 15, 2024 at 09:08:47AM -0700, Casey Schaufler wrote: > > > > On 3/15/2024 8:02 AM, Serge E. Hallyn wrote: > > > > > On Wed, Mar 13, 2024 at 10:22:03PM -0400, Paul Moore wrote: > > > > >> Passing a NULL buffer into the lsm_get_self_attr() syscall is a valid > > > > >> way to quickly determine the minimum size of the buffer needed to for > > > > >> the syscall to return all of the LSM attributes to the caller. > > > > >> Unfortunately we/I broke that behavior in commit d7cf3412a9f6 > > > > >> ("lsm: consolidate buffer size handling into lsm_fill_user_ctx()") > > > > >> such that it returned an error to the caller; this patch restores the > > > > >> original desired behavior of using the NULL buffer as a quick way to > > > > >> correctly size the attribute buffer. > > > > >> > > > > >> Cc: stable@vger.kernel.org > > > > >> Fixes: d7cf3412a9f6 ("lsm: consolidate buffer size handling into lsm_fill_user_ctx()") > > > > >> Signed-off-by: Paul Moore <paul@paul-moore.com> > > > > >> --- > > > > >> security/security.c | 8 +++++++- > > > > >> 1 file changed, 7 insertions(+), 1 deletion(-) > > > > >> > > > > >> diff --git a/security/security.c b/security/security.c > > > > >> index 5b2e0a15377d..7e118858b545 100644 > > > > >> --- a/security/security.c > > > > >> +++ b/security/security.c > > > > >> @@ -780,7 +780,9 @@ static int lsm_superblock_alloc(struct super_block *sb) > > > > >> * @id: LSM id > > > > >> * @flags: LSM defined flags > > > > >> * > > > > >> - * Fill all of the fields in a userspace lsm_ctx structure. > > > > >> + * Fill all of the fields in a userspace lsm_ctx structure. If @uctx is NULL > > > > >> + * simply calculate the required size to output via @utc_len and return > > > > >> + * success. > > > > >> * > > > > >> * Returns 0 on success, -E2BIG if userspace buffer is not large enough, > > > > >> * -EFAULT on a copyout error, -ENOMEM if memory can't be allocated. > > > > >> @@ -799,6 +801,10 @@ int lsm_fill_user_ctx(struct lsm_ctx __user *uctx, u32 *uctx_len, > > > > >> goto out; > > > > >> } > > > > >> > > > > >> + /* no buffer - return success/0 and set @uctx_len to the req size */ > > > > >> + if (!uctx) > > > > >> + goto out; > > > > > If the user just passes in *uctx_len=0, then they will get -E2BIG > > > > > but still will get the length in *uctx_len. > > > > > > > > Yes. > > > > > > > > > To use it this new way, they have to first set *uctx_len to a > > > > > value larger than nctx_len could possibly be, else they'll... > > > > > still get -E2BIG. > > > > > > > > Not sure I understand the problem. A return of 0 or E2BIG gets the > > > > caller the size. > > > > > > The problem is that there are two ways of doing the same thing, with > > > different behavior. People are bound to get it wrong at some point, > > > and it's more corner cases to try and maintain (once we start). > > > > I have a different perspective on this, you can supply either a NULL > > buffer and/or a buffer that is too small, including a size of zero, > > and you'll get back an -E2BIG and a minimum buffer size. What's the > > old wisdom, be conservative in what you send and liberal in what you > > accept? > > But if you pass a NULL uctx, then surely you should send *uctx_len=0. > And that is already handled. Why should we assume that userspace is always going to behave a certain way? Userspace is going to do crazy stuff, that's a given, I just want to make sure that we protect ourselves against the really crazy stuff, and if we can do something useful with the moderately crazy stuff I think we should. > What you are adding is that the user can pass NULL uctx, but a large > uctx_len value. > > Perhaps should change my objection, and say that I would prefer the > comment fix to suggest passing in uctx_len=0 and uctx=NULL, and expect > a -E2BIG. The NULL check can stay (though I still think it should > return -E2BIG). > > Because with the current comment update, the user may pass in > uctx=NULL, but the actual rv will change between 0 and -E2BIG > depending on the uctx_len they sent in. Which is whack, since > the incoming value means nothing. I think that's a desirable behavior, if you pass in a NULL buffer we'll provide you with the required size and return -E2BIG if the size you gave us was too small, and zero/success if the size you provided was adequate. Maybe I'm being stupid and this really is "whack", but you've got to help me understand what harm can come from the behavior above.
On Fri, Mar 15, 2024 at 12:42:27PM -0400, Paul Moore wrote: > On Fri, Mar 15, 2024 at 12:28 PM Serge E. Hallyn <serge@hallyn.com> wrote: > > On Fri, Mar 15, 2024 at 12:19:05PM -0400, Paul Moore wrote: > > > On Fri, Mar 15, 2024 at 12:13 PM Serge E. Hallyn <serge@hallyn.com> wrote: > > > > On Fri, Mar 15, 2024 at 09:08:47AM -0700, Casey Schaufler wrote: > > > > > On 3/15/2024 8:02 AM, Serge E. Hallyn wrote: > > > > > > On Wed, Mar 13, 2024 at 10:22:03PM -0400, Paul Moore wrote: > > > > > >> Passing a NULL buffer into the lsm_get_self_attr() syscall is a valid > > > > > >> way to quickly determine the minimum size of the buffer needed to for > > > > > >> the syscall to return all of the LSM attributes to the caller. > > > > > >> Unfortunately we/I broke that behavior in commit d7cf3412a9f6 > > > > > >> ("lsm: consolidate buffer size handling into lsm_fill_user_ctx()") > > > > > >> such that it returned an error to the caller; this patch restores the > > > > > >> original desired behavior of using the NULL buffer as a quick way to > > > > > >> correctly size the attribute buffer. > > > > > >> > > > > > >> Cc: stable@vger.kernel.org > > > > > >> Fixes: d7cf3412a9f6 ("lsm: consolidate buffer size handling into lsm_fill_user_ctx()") > > > > > >> Signed-off-by: Paul Moore <paul@paul-moore.com> > > > > > >> --- > > > > > >> security/security.c | 8 +++++++- > > > > > >> 1 file changed, 7 insertions(+), 1 deletion(-) > > > > > >> > > > > > >> diff --git a/security/security.c b/security/security.c > > > > > >> index 5b2e0a15377d..7e118858b545 100644 > > > > > >> --- a/security/security.c > > > > > >> +++ b/security/security.c > > > > > >> @@ -780,7 +780,9 @@ static int lsm_superblock_alloc(struct super_block *sb) > > > > > >> * @id: LSM id > > > > > >> * @flags: LSM defined flags > > > > > >> * > > > > > >> - * Fill all of the fields in a userspace lsm_ctx structure. > > > > > >> + * Fill all of the fields in a userspace lsm_ctx structure. If @uctx is NULL > > > > > >> + * simply calculate the required size to output via @utc_len and return > > > > > >> + * success. > > > > > >> * > > > > > >> * Returns 0 on success, -E2BIG if userspace buffer is not large enough, > > > > > >> * -EFAULT on a copyout error, -ENOMEM if memory can't be allocated. > > > > > >> @@ -799,6 +801,10 @@ int lsm_fill_user_ctx(struct lsm_ctx __user *uctx, u32 *uctx_len, > > > > > >> goto out; > > > > > >> } > > > > > >> > > > > > >> + /* no buffer - return success/0 and set @uctx_len to the req size */ > > > > > >> + if (!uctx) > > > > > >> + goto out; > > > > > > If the user just passes in *uctx_len=0, then they will get -E2BIG > > > > > > but still will get the length in *uctx_len. > > > > > > > > > > Yes. > > > > > > > > > > > To use it this new way, they have to first set *uctx_len to a > > > > > > value larger than nctx_len could possibly be, else they'll... > > > > > > still get -E2BIG. > > > > > > > > > > Not sure I understand the problem. A return of 0 or E2BIG gets the > > > > > caller the size. > > > > > > > > The problem is that there are two ways of doing the same thing, with > > > > different behavior. People are bound to get it wrong at some point, > > > > and it's more corner cases to try and maintain (once we start). > > > > > > I have a different perspective on this, you can supply either a NULL > > > buffer and/or a buffer that is too small, including a size of zero, > > > and you'll get back an -E2BIG and a minimum buffer size. What's the > > > old wisdom, be conservative in what you send and liberal in what you > > > accept? > > > > But if you pass a NULL uctx, then surely you should send *uctx_len=0. > > And that is already handled. > > Why should we assume that userspace is always going to behave a > certain way? Userspace is going to do crazy stuff, that's a given, I > just want to make sure that we protect ourselves against the really > crazy stuff, and if we can do something useful with the moderately > crazy stuff I think we should. > > > What you are adding is that the user can pass NULL uctx, but a large > > uctx_len value. > > > > Perhaps should change my objection, and say that I would prefer the > > comment fix to suggest passing in uctx_len=0 and uctx=NULL, and expect > > a -E2BIG. The NULL check can stay (though I still think it should > > return -E2BIG). > > > > Because with the current comment update, the user may pass in > > uctx=NULL, but the actual rv will change between 0 and -E2BIG > > depending on the uctx_len they sent in. Which is whack, since > > the incoming value means nothing. > > I think that's a desirable behavior, if you pass in a NULL buffer > we'll provide you with the required size and return -E2BIG if the size > you gave us was too small, and zero/success if the size you provided > was adequate. > > Maybe I'm being stupid and this really is "whack", but you've got to > help me understand what harm can come from the behavior above. Returning success if uctx==NULL and uctx_len is big enough sends the message that this -sending a non-zero length and uctx=NULL is a recommended use case. It should not be a recommended use. If the user wants to find out the size of the buffer, they can already do so more reliably in the pre-existing way, passing in a *length=0 (and a NULL or random uctx). I say more reliable, because they can predict the function return value in this case: -E2BIG. So the only way uctx==NULL and uctx_len!=0 should happen is by accident, and in that case sending back 'success' is misleading. > ... What's the > old wisdom, be conservative in what you send and liberal in what you > accept? If you've received garbage, you should let the sender know, rather than return 'success' in the hopes that it wasn't important. But anyway I'll stop here - it doesn't break anything directly, I just think it's a bad API with the potential for future harder to spot bugs. -serge
On Fri, Mar 15, 2024 at 1:00 PM Serge E. Hallyn <serge@hallyn.com> wrote: > On Fri, Mar 15, 2024 at 12:42:27PM -0400, Paul Moore wrote: > > On Fri, Mar 15, 2024 at 12:28 PM Serge E. Hallyn <serge@hallyn.com> wrote: > > > On Fri, Mar 15, 2024 at 12:19:05PM -0400, Paul Moore wrote: > > > > On Fri, Mar 15, 2024 at 12:13 PM Serge E. Hallyn <serge@hallyn.com> wrote: > > > > > On Fri, Mar 15, 2024 at 09:08:47AM -0700, Casey Schaufler wrote: > > > > > > On 3/15/2024 8:02 AM, Serge E. Hallyn wrote: > > > > > > > On Wed, Mar 13, 2024 at 10:22:03PM -0400, Paul Moore wrote: > > > > > > >> Passing a NULL buffer into the lsm_get_self_attr() syscall is a valid > > > > > > >> way to quickly determine the minimum size of the buffer needed to for > > > > > > >> the syscall to return all of the LSM attributes to the caller. > > > > > > >> Unfortunately we/I broke that behavior in commit d7cf3412a9f6 > > > > > > >> ("lsm: consolidate buffer size handling into lsm_fill_user_ctx()") > > > > > > >> such that it returned an error to the caller; this patch restores the > > > > > > >> original desired behavior of using the NULL buffer as a quick way to > > > > > > >> correctly size the attribute buffer. > > > > > > >> > > > > > > >> Cc: stable@vger.kernel.org > > > > > > >> Fixes: d7cf3412a9f6 ("lsm: consolidate buffer size handling into lsm_fill_user_ctx()") > > > > > > >> Signed-off-by: Paul Moore <paul@paul-moore.com> > > > > > > >> --- > > > > > > >> security/security.c | 8 +++++++- > > > > > > >> 1 file changed, 7 insertions(+), 1 deletion(-) > > > > > > >> > > > > > > >> diff --git a/security/security.c b/security/security.c > > > > > > >> index 5b2e0a15377d..7e118858b545 100644 > > > > > > >> --- a/security/security.c > > > > > > >> +++ b/security/security.c > > > > > > >> @@ -780,7 +780,9 @@ static int lsm_superblock_alloc(struct super_block *sb) > > > > > > >> * @id: LSM id > > > > > > >> * @flags: LSM defined flags > > > > > > >> * > > > > > > >> - * Fill all of the fields in a userspace lsm_ctx structure. > > > > > > >> + * Fill all of the fields in a userspace lsm_ctx structure. If @uctx is NULL > > > > > > >> + * simply calculate the required size to output via @utc_len and return > > > > > > >> + * success. > > > > > > >> * > > > > > > >> * Returns 0 on success, -E2BIG if userspace buffer is not large enough, > > > > > > >> * -EFAULT on a copyout error, -ENOMEM if memory can't be allocated. > > > > > > >> @@ -799,6 +801,10 @@ int lsm_fill_user_ctx(struct lsm_ctx __user *uctx, u32 *uctx_len, > > > > > > >> goto out; > > > > > > >> } > > > > > > >> > > > > > > >> + /* no buffer - return success/0 and set @uctx_len to the req size */ > > > > > > >> + if (!uctx) > > > > > > >> + goto out; > > > > > > > If the user just passes in *uctx_len=0, then they will get -E2BIG > > > > > > > but still will get the length in *uctx_len. > > > > > > > > > > > > Yes. > > > > > > > > > > > > > To use it this new way, they have to first set *uctx_len to a > > > > > > > value larger than nctx_len could possibly be, else they'll... > > > > > > > still get -E2BIG. > > > > > > > > > > > > Not sure I understand the problem. A return of 0 or E2BIG gets the > > > > > > caller the size. > > > > > > > > > > The problem is that there are two ways of doing the same thing, with > > > > > different behavior. People are bound to get it wrong at some point, > > > > > and it's more corner cases to try and maintain (once we start). > > > > > > > > I have a different perspective on this, you can supply either a NULL > > > > buffer and/or a buffer that is too small, including a size of zero, > > > > and you'll get back an -E2BIG and a minimum buffer size. What's the > > > > old wisdom, be conservative in what you send and liberal in what you > > > > accept? > > > > > > But if you pass a NULL uctx, then surely you should send *uctx_len=0. > > > And that is already handled. > > > > Why should we assume that userspace is always going to behave a > > certain way? Userspace is going to do crazy stuff, that's a given, I > > just want to make sure that we protect ourselves against the really > > crazy stuff, and if we can do something useful with the moderately > > crazy stuff I think we should. > > > > > What you are adding is that the user can pass NULL uctx, but a large > > > uctx_len value. > > > > > > Perhaps should change my objection, and say that I would prefer the > > > comment fix to suggest passing in uctx_len=0 and uctx=NULL, and expect > > > a -E2BIG. The NULL check can stay (though I still think it should > > > return -E2BIG). > > > > > > Because with the current comment update, the user may pass in > > > uctx=NULL, but the actual rv will change between 0 and -E2BIG > > > depending on the uctx_len they sent in. Which is whack, since > > > the incoming value means nothing. > > > > I think that's a desirable behavior, if you pass in a NULL buffer > > we'll provide you with the required size and return -E2BIG if the size > > you gave us was too small, and zero/success if the size you provided > > was adequate. > > > > Maybe I'm being stupid and this really is "whack", but you've got to > > help me understand what harm can come from the behavior above. > > Returning success if uctx==NULL and uctx_len is big enough sends the > message that this -sending a non-zero length and uctx=NULL is a > recommended use case. It should not be a recommended use. I would argue that simply tolerating a given input and recommending its use are two different things. I'm not sure we're officially recommending any particular usage, we just describe how the syscall works and we let userspace do as they see fit within those constraints. > If the user wants to find out the size of the buffer, they can already > do so more reliably in the pre-existing way, passing in a *length=0 (and > a NULL or random uctx). I say more reliable, because they can predict > the function return value in this case: -E2BIG. As you well know, there is nothing preventing that use case. > So the only way uctx==NULL and uctx_len!=0 should happen is by accident, > and in that case sending back 'success' is misleading. Noted, but I disagree primarily on the grounds that this implies both a preference for a particular use case (see above) as well as the impossibility of understanding the intention behind all of the userspace applications that may make use of this in the future. > > ... What's the > > old wisdom, be conservative in what you send and liberal in what you > > accept? > > If you've received garbage, you should let the sender know, rather than > return 'success' in the hopes that it wasn't important. > > But anyway I'll stop here - it doesn't break anything directly, I just > think it's a bad API with the potential for future harder to spot bugs. Fair enough, thank you for the review and debate. For others tuning into this thread, I believe it is important to mention that this patch restored an API behavior that had been in existence for several months (all?) of the original patchset's life; the best time to suggest changes to that behavior was during those ~16 months. If we were to change the API behavior now, we would need to have some demonstrated harm, e.g. 32-bit/64-bit ABI incompatibility, to consider a change.
On Fri, Mar 15, 2024 at 03:32:02PM -0400, Paul Moore wrote: > On Fri, Mar 15, 2024 at 1:00 PM Serge E. Hallyn <serge@hallyn.com> wrote: > > On Fri, Mar 15, 2024 at 12:42:27PM -0400, Paul Moore wrote: > > > On Fri, Mar 15, 2024 at 12:28 PM Serge E. Hallyn <serge@hallyn.com> wrote: > > > > On Fri, Mar 15, 2024 at 12:19:05PM -0400, Paul Moore wrote: > > > > > On Fri, Mar 15, 2024 at 12:13 PM Serge E. Hallyn <serge@hallyn.com> wrote: > > > > > > On Fri, Mar 15, 2024 at 09:08:47AM -0700, Casey Schaufler wrote: > > > > > > > On 3/15/2024 8:02 AM, Serge E. Hallyn wrote: > > > > > > > > On Wed, Mar 13, 2024 at 10:22:03PM -0400, Paul Moore wrote: > > > > > > > >> Passing a NULL buffer into the lsm_get_self_attr() syscall is a valid > > > > > > > >> way to quickly determine the minimum size of the buffer needed to for > > > > > > > >> the syscall to return all of the LSM attributes to the caller. > > > > > > > >> Unfortunately we/I broke that behavior in commit d7cf3412a9f6 > > > > > > > >> ("lsm: consolidate buffer size handling into lsm_fill_user_ctx()") > > > > > > > >> such that it returned an error to the caller; this patch restores the > > > > > > > >> original desired behavior of using the NULL buffer as a quick way to > > > > > > > >> correctly size the attribute buffer. > > > > > > > >> > > > > > > > >> Cc: stable@vger.kernel.org > > > > > > > >> Fixes: d7cf3412a9f6 ("lsm: consolidate buffer size handling into lsm_fill_user_ctx()") > > > > > > > >> Signed-off-by: Paul Moore <paul@paul-moore.com> > > > > > > > >> --- > > > > > > > >> security/security.c | 8 +++++++- > > > > > > > >> 1 file changed, 7 insertions(+), 1 deletion(-) > > > > > > > >> > > > > > > > >> diff --git a/security/security.c b/security/security.c > > > > > > > >> index 5b2e0a15377d..7e118858b545 100644 > > > > > > > >> --- a/security/security.c > > > > > > > >> +++ b/security/security.c > > > > > > > >> @@ -780,7 +780,9 @@ static int lsm_superblock_alloc(struct super_block *sb) > > > > > > > >> * @id: LSM id > > > > > > > >> * @flags: LSM defined flags > > > > > > > >> * > > > > > > > >> - * Fill all of the fields in a userspace lsm_ctx structure. > > > > > > > >> + * Fill all of the fields in a userspace lsm_ctx structure. If @uctx is NULL > > > > > > > >> + * simply calculate the required size to output via @utc_len and return > > > > > > > >> + * success. > > > > > > > >> * > > > > > > > >> * Returns 0 on success, -E2BIG if userspace buffer is not large enough, > > > > > > > >> * -EFAULT on a copyout error, -ENOMEM if memory can't be allocated. > > > > > > > >> @@ -799,6 +801,10 @@ int lsm_fill_user_ctx(struct lsm_ctx __user *uctx, u32 *uctx_len, > > > > > > > >> goto out; > > > > > > > >> } > > > > > > > >> > > > > > > > >> + /* no buffer - return success/0 and set @uctx_len to the req size */ > > > > > > > >> + if (!uctx) > > > > > > > >> + goto out; > > > > > > > > If the user just passes in *uctx_len=0, then they will get -E2BIG > > > > > > > > but still will get the length in *uctx_len. > > > > > > > > > > > > > > Yes. > > > > > > > > > > > > > > > To use it this new way, they have to first set *uctx_len to a > > > > > > > > value larger than nctx_len could possibly be, else they'll... > > > > > > > > still get -E2BIG. > > > > > > > > > > > > > > Not sure I understand the problem. A return of 0 or E2BIG gets the > > > > > > > caller the size. > > > > > > > > > > > > The problem is that there are two ways of doing the same thing, with > > > > > > different behavior. People are bound to get it wrong at some point, > > > > > > and it's more corner cases to try and maintain (once we start). > > > > > > > > > > I have a different perspective on this, you can supply either a NULL > > > > > buffer and/or a buffer that is too small, including a size of zero, > > > > > and you'll get back an -E2BIG and a minimum buffer size. What's the > > > > > old wisdom, be conservative in what you send and liberal in what you > > > > > accept? > > > > > > > > But if you pass a NULL uctx, then surely you should send *uctx_len=0. > > > > And that is already handled. > > > > > > Why should we assume that userspace is always going to behave a > > > certain way? Userspace is going to do crazy stuff, that's a given, I > > > just want to make sure that we protect ourselves against the really > > > crazy stuff, and if we can do something useful with the moderately > > > crazy stuff I think we should. > > > > > > > What you are adding is that the user can pass NULL uctx, but a large > > > > uctx_len value. > > > > > > > > Perhaps should change my objection, and say that I would prefer the > > > > comment fix to suggest passing in uctx_len=0 and uctx=NULL, and expect > > > > a -E2BIG. The NULL check can stay (though I still think it should > > > > return -E2BIG). > > > > > > > > Because with the current comment update, the user may pass in > > > > uctx=NULL, but the actual rv will change between 0 and -E2BIG > > > > depending on the uctx_len they sent in. Which is whack, since > > > > the incoming value means nothing. > > > > > > I think that's a desirable behavior, if you pass in a NULL buffer > > > we'll provide you with the required size and return -E2BIG if the size > > > you gave us was too small, and zero/success if the size you provided > > > was adequate. > > > > > > Maybe I'm being stupid and this really is "whack", but you've got to > > > help me understand what harm can come from the behavior above. > > > > Returning success if uctx==NULL and uctx_len is big enough sends the > > message that this -sending a non-zero length and uctx=NULL is a > > recommended use case. It should not be a recommended use. > > I would argue that simply tolerating a given input and recommending > its use are two different things. I'm not sure we're officially > recommending any particular usage, we just describe how the syscall > works and we let userspace do as they see fit within those > constraints. It would be good to make clear, concise recommendations - but that's probably for the lsm-syscalls library to do. > > If the user wants to find out the size of the buffer, they can already > > do so more reliably in the pre-existing way, passing in a *length=0 (and > > a NULL or random uctx). I say more reliable, because they can predict > > the function return value in this case: -E2BIG. > > As you well know, there is nothing preventing that use case. Of course. > > So the only way uctx==NULL and uctx_len!=0 should happen is by accident, > > and in that case sending back 'success' is misleading. > > Noted, but I disagree primarily on the grounds that this implies both > a preference for a particular use case (see above) as well as the > impossibility of understanding the intention behind all of the > userspace applications that may make use of this in the future. > > > > ... What's the > > > old wisdom, be conservative in what you send and liberal in what you > > > accept? > > > > If you've received garbage, you should let the sender know, rather than > > return 'success' in the hopes that it wasn't important. > > > > But anyway I'll stop here - it doesn't break anything directly, I just > > think it's a bad API with the potential for future harder to spot bugs. > > Fair enough, thank you for the review and debate. I came here for an argument! > For others tuning into this thread, I believe it is important to > mention that this patch restored an API behavior that had been in > existence for several months (all?) of the original patchset's life; > the best time to suggest changes to that behavior was during those ~16 > months. If we were to change the API behavior now, we would need to > have some demonstrated harm, e.g. 32-bit/64-bit ABI incompatibility, > to consider a change. Ah, I wasn't aware of that. I should have guessed it when you said this broke a testcase. Then yeah best to restore the previous behavior. Acked-by: Serge Hallyn <serge@hallyn.com> just for the record. thanks, -serge
diff --git a/security/security.c b/security/security.c index 5b2e0a15377d..7e118858b545 100644 --- a/security/security.c +++ b/security/security.c @@ -780,7 +780,9 @@ static int lsm_superblock_alloc(struct super_block *sb) * @id: LSM id * @flags: LSM defined flags * - * Fill all of the fields in a userspace lsm_ctx structure. + * Fill all of the fields in a userspace lsm_ctx structure. If @uctx is NULL + * simply calculate the required size to output via @utc_len and return + * success. * * Returns 0 on success, -E2BIG if userspace buffer is not large enough, * -EFAULT on a copyout error, -ENOMEM if memory can't be allocated. @@ -799,6 +801,10 @@ int lsm_fill_user_ctx(struct lsm_ctx __user *uctx, u32 *uctx_len, goto out; } + /* no buffer - return success/0 and set @uctx_len to the req size */ + if (!uctx) + goto out; + nctx = kzalloc(nctx_len, GFP_KERNEL); if (nctx == NULL) { rc = -ENOMEM;
Passing a NULL buffer into the lsm_get_self_attr() syscall is a valid way to quickly determine the minimum size of the buffer needed to for the syscall to return all of the LSM attributes to the caller. Unfortunately we/I broke that behavior in commit d7cf3412a9f6 ("lsm: consolidate buffer size handling into lsm_fill_user_ctx()") such that it returned an error to the caller; this patch restores the original desired behavior of using the NULL buffer as a quick way to correctly size the attribute buffer. Cc: stable@vger.kernel.org Fixes: d7cf3412a9f6 ("lsm: consolidate buffer size handling into lsm_fill_user_ctx()") Signed-off-by: Paul Moore <paul@paul-moore.com> --- security/security.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)