capabilities: fix sparse warning about __user access

[PATCH] capabilities: fix sparse warning about __user access

Posted by Ben Dooks 2 years, 7 months ago

The two syscalls for capget and capset are producing sparse warnings
as sparse is thinking that the "struct __user_cap_data_struct" is marked
user, which seems to be down to the declaration and typedef at the same
time.

Fix the following warnings by splutting the struct declaration and then
the user typedef into two:

kernel/capability.c:191:35: warning: incorrect type in argument 2 (different address spaces)
kernel/capability.c:191:35:    expected void const *from
kernel/capability.c:191:35:    got struct __user_cap_data_struct [noderef] __user *
kernel/capability.c:168:14: warning: dereference of noderef expression
kernel/capability.c:168:45: warning: dereference of noderef expression
kernel/capability.c:169:14: warning: dereference of noderef expression
kernel/capability.c:169:45: warning: dereference of noderef expression
kernel/capability.c:170:14: warning: dereference of noderef expression
kernel/capability.c:170:45: warning: dereference of noderef expression
kernel/capability.c:244:29: warning: incorrect type in argument 1 (different address spaces)
kernel/capability.c:244:29:    expected void *to
kernel/capability.c:244:29:    got struct __user_cap_data_struct [noderef] __user ( * )[2]
kernel/capability.c:247:42: warning: dereference of noderef expression
kernel/capability.c:247:64: warning: dereference of noderef expression
kernel/capability.c:248:42: warning: dereference of noderef expression
kernel/capability.c:248:64: warning: dereference of noderef expression
kernel/capability.c:249:42: warning: dereference of noderef expression
kernel/capability.c:249:64: warning: dereference of noderef expression

Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
---
 include/uapi/linux/capability.h | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/include/uapi/linux/capability.h b/include/uapi/linux/capability.h
index 3d61a0ae055d..5bb906098697 100644
--- a/include/uapi/linux/capability.h
+++ b/include/uapi/linux/capability.h
@@ -41,11 +41,12 @@ typedef struct __user_cap_header_struct {
 	int pid;
 } __user *cap_user_header_t;
 
-typedef struct __user_cap_data_struct {
+struct __user_cap_data_struct {
         __u32 effective;
         __u32 permitted;
         __u32 inheritable;
-} __user *cap_user_data_t;
+};
+typedef struct __user_cap_data_struct __user *cap_user_data_t;
 
 
 #define VFS_CAP_REVISION_MASK	0xFF000000
-- 
2.39.2

Re: [PATCH] capabilities: fix sparse warning about __user access

Posted by Serge E. Hallyn 2 years, 7 months ago

On Mon, Jun 19, 2023 at 01:35:35PM +0100, Ben Dooks wrote:
> The two syscalls for capget and capset are producing sparse warnings
> as sparse is thinking that the "struct __user_cap_data_struct" is marked
> user, which seems to be down to the declaration and typedef at the same
> time.
> 
> Fix the following warnings by splutting the struct declaration and then
> the user typedef into two:

I'm not a fan of making code changes to work around scanners'
shortcomings, mainly because eventually I assume the scanners
will learn to deal with it.

However, I don't like the all-in-one typedef+struct definition
either, so let's go with it :)

Paul, do you mind picking this up?

thanks,
-serge

> kernel/capability.c:191:35: warning: incorrect type in argument 2 (different address spaces)
> kernel/capability.c:191:35:    expected void const *from
> kernel/capability.c:191:35:    got struct __user_cap_data_struct [noderef] __user *
> kernel/capability.c:168:14: warning: dereference of noderef expression
> kernel/capability.c:168:45: warning: dereference of noderef expression
> kernel/capability.c:169:14: warning: dereference of noderef expression
> kernel/capability.c:169:45: warning: dereference of noderef expression
> kernel/capability.c:170:14: warning: dereference of noderef expression
> kernel/capability.c:170:45: warning: dereference of noderef expression
> kernel/capability.c:244:29: warning: incorrect type in argument 1 (different address spaces)
> kernel/capability.c:244:29:    expected void *to
> kernel/capability.c:244:29:    got struct __user_cap_data_struct [noderef] __user ( * )[2]
> kernel/capability.c:247:42: warning: dereference of noderef expression
> kernel/capability.c:247:64: warning: dereference of noderef expression
> kernel/capability.c:248:42: warning: dereference of noderef expression
> kernel/capability.c:248:64: warning: dereference of noderef expression
> kernel/capability.c:249:42: warning: dereference of noderef expression
> kernel/capability.c:249:64: warning: dereference of noderef expression
> 
> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>

Reviewed-by: Serge Hallyn <serge@hallyn.com>

> ---
>  include/uapi/linux/capability.h | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/include/uapi/linux/capability.h b/include/uapi/linux/capability.h
> index 3d61a0ae055d..5bb906098697 100644
> --- a/include/uapi/linux/capability.h
> +++ b/include/uapi/linux/capability.h
> @@ -41,11 +41,12 @@ typedef struct __user_cap_header_struct {
>  	int pid;
>  } __user *cap_user_header_t;
>  
> -typedef struct __user_cap_data_struct {
> +struct __user_cap_data_struct {
>          __u32 effective;
>          __u32 permitted;
>          __u32 inheritable;
> -} __user *cap_user_data_t;
> +};
> +typedef struct __user_cap_data_struct __user *cap_user_data_t;
>  
>  
>  #define VFS_CAP_REVISION_MASK	0xFF000000
> -- 
> 2.39.2

Re: [PATCH] capabilities: fix sparse warning about __user access

Posted by Paul Moore 2 years, 7 months ago

On Mon, Jun 19, 2023 at 1:57 PM Serge E. Hallyn <serge@hallyn.com> wrote:
>
> On Mon, Jun 19, 2023 at 01:35:35PM +0100, Ben Dooks wrote:
> > The two syscalls for capget and capset are producing sparse warnings
> > as sparse is thinking that the "struct __user_cap_data_struct" is marked
> > user, which seems to be down to the declaration and typedef at the same
> > time.
> >
> > Fix the following warnings by splutting the struct declaration and then
> > the user typedef into two:
>
> I'm not a fan of making code changes to work around scanners'
> shortcomings, mainly because eventually I assume the scanners
> will learn to deal with it.
>
> However, I don't like the all-in-one typedef+struct definition
> either, so let's go with it :)
>
> Paul, do you mind picking this up?

Sure, no problem.  Since we are at -rc7, I'm assuming this can wait
until after the merge window?

-- 
paul-moore.com

Re: [PATCH] capabilities: fix sparse warning about __user access

Posted by Serge Hallyn 2 years, 7 months ago

On Mon, Jun 19, 2023 at 05:47:54PM -0400, Paul Moore wrote:
> On Mon, Jun 19, 2023 at 1:57 PM Serge E. Hallyn <serge@hallyn.com> wrote:
> >
> > On Mon, Jun 19, 2023 at 01:35:35PM +0100, Ben Dooks wrote:
> > > The two syscalls for capget and capset are producing sparse warnings
> > > as sparse is thinking that the "struct __user_cap_data_struct" is marked
> > > user, which seems to be down to the declaration and typedef at the same
> > > time.
> > >
> > > Fix the following warnings by splutting the struct declaration and then
> > > the user typedef into two:
> >
> > I'm not a fan of making code changes to work around scanners'
> > shortcomings, mainly because eventually I assume the scanners
> > will learn to deal with it.
> >
> > However, I don't like the all-in-one typedef+struct definition
> > either, so let's go with it :)
> >
> > Paul, do you mind picking this up?
> 
> Sure, no problem.  Since we are at -rc7, I'm assuming this can wait
> until after the merge window?

Yeah, it's just fixing a sparse warning, no urgency.

thanks,
-serge

Re: [PATCH] capabilities: fix sparse warning about __user access

Posted by Paul Moore 2 years, 7 months ago

On Wed, Jun 21, 2023 at 9:43 AM Serge Hallyn <serge@hallyn.com> wrote:
> On Mon, Jun 19, 2023 at 05:47:54PM -0400, Paul Moore wrote:
> > On Mon, Jun 19, 2023 at 1:57 PM Serge E. Hallyn <serge@hallyn.com> wrote:
> > >
> > > On Mon, Jun 19, 2023 at 01:35:35PM +0100, Ben Dooks wrote:
> > > > The two syscalls for capget and capset are producing sparse warnings
> > > > as sparse is thinking that the "struct __user_cap_data_struct" is marked
> > > > user, which seems to be down to the declaration and typedef at the same
> > > > time.
> > > >
> > > > Fix the following warnings by splutting the struct declaration and then
> > > > the user typedef into two:
> > >
> > > I'm not a fan of making code changes to work around scanners'
> > > shortcomings, mainly because eventually I assume the scanners
> > > will learn to deal with it.
> > >
> > > However, I don't like the all-in-one typedef+struct definition
> > > either, so let's go with it :)
> > >
> > > Paul, do you mind picking this up?
> >
> > Sure, no problem.  Since we are at -rc7, I'm assuming this can wait
> > until after the merge window?
>
> Yeah, it's just fixing a sparse warning, no urgency.

Actually, this looks like a dup of 55382134366e ("capability: erase
checker warnings about struct __user_cap_data_struct") which is
currently in Linus' tree.

Thank you for your patch Ben, but it looks as if we had a patch a
couple of weeks before yours which fixed the same problem.  If you
notice continuing problems with the latest kernel sources from Linus
please let us know.

-- 
paul-moore.com