[PATCH] staging: lustre: lent: add missing __user __user annotation

[PATCH] staging: lustre: lent: add missing __user __user annotation

[Ioana Ciornei]

Add missing __user annotation when using copy_{to/from}_user() to indicate that is
a pointer from userspace and that it should not be derefenced.
Also, remove sparse warnings such as:

drivers/staging/lustre/lnet/selftest/console.c:1474:26: warning: incorrect type in argument 1 (different address spaces)
  1 drivers/staging/lustre/lnet/selftest/console.c:1474:26:    expected void [noderef] <asn:1>*to
  2 drivers/staging/lustre/lnet/selftest/console.c:1474:26:    got struct sfw_counters_t [usertype] *[assigned] sfwk_stat

Signed-off-by: Ioana Ciornei <ciorneiioana@gmail.com>
---
 drivers/staging/lustre/lnet/selftest/console.c | 44 +++++++++++++-------------
 1 file changed, 22 insertions(+), 22 deletions(-)

diff --git a/drivers/staging/lustre/lnet/selftest/console.c b/drivers/staging/lustre/lnet/selftest/console.c
index 024aaee..7ba9022 100644
--- a/drivers/staging/lustre/lnet/selftest/console.c
+++ b/drivers/staging/lustre/lnet/selftest/console.c
@@ -384,9 +384,9 @@ lstcon_sesrpc_readent(int transop, srpc_msg_t *msg,
 	case LST_TRANS_SESQRY:
 		rep = &msg->msg_body.dbg_reply;
 
-		if (copy_to_user(&ent_up->rpe_priv[0],
+		if (copy_to_user((void __user *)&ent_up->rpe_priv[0],
 				     &rep->dbg_timeout, sizeof(int)) ||
-		    copy_to_user(&ent_up->rpe_payload[0],
+		    copy_to_user((void __user *)&ent_up->rpe_payload[0],
 				     &rep->dbg_name, LST_NAME_SIZE))
 			return -EFAULT;
 
@@ -418,7 +418,7 @@ lstcon_group_nodes_add(lstcon_group_t *grp,
 	}
 
 	for (i = 0 ; i < count; i++) {
-		if (copy_from_user(&id, &ids_up[i], sizeof(id))) {
+		if (copy_from_user(&id, (void __user *)&ids_up[i], sizeof(id))) {
 			rc = -EFAULT;
 			break;
 		}
@@ -487,7 +487,7 @@ lstcon_group_nodes_remove(lstcon_group_t *grp,
 	}
 
 	for (i = 0; i < count; i++) {
-		if (copy_from_user(&id, &ids_up[i], sizeof(id))) {
+		if (copy_from_user(&id, (void __user *)&ids_up[i], sizeof(id))) {
 			rc = -EFAULT;
 			goto error;
 		}
@@ -731,7 +731,7 @@ lstcon_group_list(int index, int len, char *name_up)
 
 	list_for_each_entry(grp, &console_session.ses_grp_list, grp_link) {
 		if (index-- == 0) {
-			return copy_to_user(name_up, grp->grp_name, len) ?
+			return copy_to_user((void __user *)name_up, grp->grp_name, len) ?
 			       -EFAULT : 0;
 		}
 	}
@@ -761,9 +761,9 @@ lstcon_nodes_getent(struct list_head *head, int *index_p,
 			break;
 
 		nd = ndl->ndl_node;
-		if (copy_to_user(&dents_up[count].nde_id,
+		if (copy_to_user((void __user *)&dents_up[count].nde_id,
 				     &nd->nd_id, sizeof(nd->nd_id)) ||
-		    copy_to_user(&dents_up[count].nde_state,
+		    copy_to_user((void __user *)&dents_up[count].nde_state,
 				     &nd->nd_state, sizeof(nd->nd_state)))
 			return -EFAULT;
 
@@ -815,7 +815,7 @@ lstcon_group_info(char *name, lstcon_ndlist_ent_t *gents_p,
 	list_for_each_entry(ndl, &grp->grp_ndl_list, ndl_link)
 		LST_NODE_STATE_COUNTER(ndl->ndl_node, gentp);
 
-	rc = copy_to_user(gents_p, gentp,
+	rc = copy_to_user((void __user *)gents_p, gentp,
 			      sizeof(lstcon_ndlist_ent_t)) ? -EFAULT : 0;
 
 	LIBCFS_FREE(gentp, sizeof(lstcon_ndlist_ent_t));
@@ -910,7 +910,7 @@ lstcon_batch_list(int index, int len, char *name_up)
 
 	list_for_each_entry(bat, &console_session.ses_bat_list, bat_link) {
 		if (index-- == 0) {
-			return copy_to_user(name_up, bat->bat_name, len) ?
+			return copy_to_user((void __user *)name_up, bat->bat_name, len) ?
 			       -EFAULT : 0;
 		}
 	}
@@ -983,7 +983,7 @@ lstcon_batch_info(char *name, lstcon_test_batch_ent_t *ent_up, int server,
 	list_for_each_entry(ndl, srvlst, ndl_link)
 		LST_NODE_STATE_COUNTER(ndl->ndl_node, &entp->tbe_srv_nle);
 
-	rc = copy_to_user(ent_up, entp,
+	rc = copy_to_user((void __user *)ent_up, entp,
 			      sizeof(lstcon_test_batch_ent_t)) ? -EFAULT : 0;
 
 	LIBCFS_FREE(entp, sizeof(lstcon_test_batch_ent_t));
@@ -1386,7 +1386,7 @@ lstcon_tsbrpc_readent(int transop, srpc_msg_t *msg,
 		 transop == LST_TRANS_TSBSRVQRY);
 
 	/* positive errno, framework error code */
-	if (copy_to_user(&ent_up->rpe_priv[0],
+	if (copy_to_user((void __user *)&ent_up->rpe_priv[0],
 			     &rep->bar_active, sizeof(rep->bar_active)))
 		return -EFAULT;
 
@@ -1471,9 +1471,9 @@ lstcon_statrpc_readent(int transop, srpc_msg_t *msg,
 	srpc_stat = (srpc_counters_t *)((char *)sfwk_stat + sizeof(*sfwk_stat));
 	lnet_stat = (lnet_counters_t *)((char *)srpc_stat + sizeof(*srpc_stat));
 
-	if (copy_to_user(sfwk_stat, &rep->str_fw, sizeof(*sfwk_stat)) ||
-	    copy_to_user(srpc_stat, &rep->str_rpc, sizeof(*srpc_stat)) ||
-	    copy_to_user(lnet_stat, &rep->str_lnet, sizeof(*lnet_stat)))
+	if (copy_to_user((void __user *)sfwk_stat, &rep->str_fw, sizeof(*sfwk_stat)) ||
+	    copy_to_user((void __user *)srpc_stat, &rep->str_rpc, sizeof(*srpc_stat)) ||
+	    copy_to_user((void __user *)lnet_stat, &rep->str_lnet, sizeof(*lnet_stat)))
 		return -EFAULT;
 
 	return 0;
@@ -1541,7 +1541,7 @@ lstcon_nodes_stat(int count, lnet_process_id_t *ids_up,
 	}
 
 	for (i = 0 ; i < count; i++) {
-		if (copy_from_user(&id, &ids_up[i], sizeof(id))) {
+		if (copy_from_user(&id, (void __user *)&ids_up[i], sizeof(id))) {
 			rc = -EFAULT;
 			break;
 		}
@@ -1653,7 +1653,7 @@ lstcon_nodes_debug(int timeout,
 	}
 
 	for (i = 0; i < count; i++) {
-		if (copy_from_user(&id, &ids_up[i], sizeof(id))) {
+		if (copy_from_user((void *)&id, (void __user *)&ids_up[i], sizeof(id))) {
 			rc = -EFAULT;
 			break;
 		}
@@ -1756,7 +1756,7 @@ lstcon_session_new(char *name, int key, unsigned feats,
 		return rc;
 	}
 
-	if (copy_to_user(sid_up, &console_session.ses_id,
+	if (copy_to_user((void __user *)sid_up, &console_session.ses_id,
 			     sizeof(lst_sid_t)) == 0)
 		return rc;
 
@@ -1783,14 +1783,14 @@ lstcon_session_info(lst_sid_t *sid_up, int *key_up, unsigned *featp,
 	list_for_each_entry(ndl, &console_session.ses_ndl_list, ndl_link)
 		LST_NODE_STATE_COUNTER(ndl->ndl_node, entp);
 
-	if (copy_to_user(sid_up, &console_session.ses_id,
+	if (copy_to_user((void __user *)sid_up, &console_session.ses_id,
 			     sizeof(lst_sid_t)) ||
-	    copy_to_user(key_up, &console_session.ses_key,
+	    copy_to_user((void __user *)key_up, &console_session.ses_key,
 			     sizeof(*key_up)) ||
-	    copy_to_user(featp, &console_session.ses_features,
+	    copy_to_user((void __user *)featp, &console_session.ses_features,
 			     sizeof(*featp)) ||
-	    copy_to_user(ndinfo_up, entp, sizeof(*entp)) ||
-	    copy_to_user(name_up, console_session.ses_name, len))
+	    copy_to_user((void __user *)ndinfo_up, entp, sizeof(*entp)) ||
+	    copy_to_user((void __user *)name_up, console_session.ses_name, len))
 		rc = -EFAULT;
 
 	LIBCFS_FREE(entp, sizeof(*entp));
-- 
2.1.4

Re: [Outreachy kernel] [PATCH] staging: lustre: lent: add missing __user __user annotation

[Arnd Bergmann]

On Friday 09 October 2015 00:30:13 Ioana Ciornei wrote:
> Add missing __user annotation when using copy_{to/from}_user() to indicate that is
> a pointer from userspace and that it should not be derefenced.
> Also, remove sparse warnings such as:
> 
> drivers/staging/lustre/lnet/selftest/console.c:1474:26: warning: incorrect type in argument 1 (different address spaces)
>   1 drivers/staging/lustre/lnet/selftest/console.c:1474:26:    expected void [noderef] <asn:1>*to
>   2 drivers/staging/lustre/lnet/selftest/console.c:1474:26:    got struct sfw_counters_t [usertype] *[assigned] sfwk_stat
> 
> Signed-off-by: Ioana Ciornei <ciorneiioana@gmail.com>

Your subject line has a duplicate "__user" in it.

> ---
>  drivers/staging/lustre/lnet/selftest/console.c | 44 +++++++++++++-------------
>  1 file changed, 22 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/staging/lustre/lnet/selftest/console.c b/drivers/staging/lustre/lnet/selftest/console.c
> index 024aaee..7ba9022 100644
> --- a/drivers/staging/lustre/lnet/selftest/console.c
> +++ b/drivers/staging/lustre/lnet/selftest/console.c
> @@ -384,9 +384,9 @@ lstcon_sesrpc_readent(int transop, srpc_msg_t *msg,
>  	case LST_TRANS_SESQRY:
>  		rep = &msg->msg_body.dbg_reply;
>  
> -		if (copy_to_user(&ent_up->rpe_priv[0],
> +		if (copy_to_user((void __user *)&ent_up->rpe_priv[0],
>  				     &rep->dbg_timeout, sizeof(int)) ||
> -		    copy_to_user(&ent_up->rpe_payload[0],
> +		    copy_to_user((void __user *)&ent_up->rpe_payload[0],
>  				     &rep->dbg_name, LST_NAME_SIZE))
>  			return -EFAULT;
>  

This looks wrong. I have not checked what is actually going on here, but
you should never add a __user cast to a variable that is accessed
through a kernel pointer. Can you find out where the ent_up variable
is and why it is not marked __user itself?

	Arnd

Re: [Outreachy kernel] [PATCH] staging: lustre: lent: add missing __user __user annotation

[Ioana Ciornei]

On Fri, Oct 9, 2015 at 12:34 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Friday 09 October 2015 00:30:13 Ioana Ciornei wrote:
>> Add missing __user annotation when using copy_{to/from}_user() to indicate that is
>> a pointer from userspace and that it should not be derefenced.
>> Also, remove sparse warnings such as:
>>
>> drivers/staging/lustre/lnet/selftest/console.c:1474:26: warning: incorrect type in argument 1 (different address spaces)
>>   1 drivers/staging/lustre/lnet/selftest/console.c:1474:26:    expected void [noderef] <asn:1>*to
>>   2 drivers/staging/lustre/lnet/selftest/console.c:1474:26:    got struct sfw_counters_t [usertype] *[assigned] sfwk_stat
>>
>> Signed-off-by: Ioana Ciornei <ciorneiioana@gmail.com>
>
> Your subject line has a duplicate "__user" in it.

Sorry about that.

>
>> ---
>>  drivers/staging/lustre/lnet/selftest/console.c | 44 +++++++++++++-------------
>>  1 file changed, 22 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/staging/lustre/lnet/selftest/console.c b/drivers/staging/lustre/lnet/selftest/console.c
>> index 024aaee..7ba9022 100644
>> --- a/drivers/staging/lustre/lnet/selftest/console.c
>> +++ b/drivers/staging/lustre/lnet/selftest/console.c
>> @@ -384,9 +384,9 @@ lstcon_sesrpc_readent(int transop, srpc_msg_t *msg,
>>       case LST_TRANS_SESQRY:
>>               rep = &msg->msg_body.dbg_reply;
>>
>> -             if (copy_to_user(&ent_up->rpe_priv[0],
>> +             if (copy_to_user((void __user *)&ent_up->rpe_priv[0],
>>                                    &rep->dbg_timeout, sizeof(int)) ||
>> -                 copy_to_user(&ent_up->rpe_payload[0],
>> +                 copy_to_user((void __user *)&ent_up->rpe_payload[0],
>>                                    &rep->dbg_name, LST_NAME_SIZE))
>>                       return -EFAULT;
>>
>
> This looks wrong. I have not checked what is actually going on here, but
> you should never add a __user cast to a variable that is accessed
> through a kernel pointer. Can you find out where the ent_up variable
> is and why it is not marked __user itself?
>

I searched a bit through the source code and the hierarchy of calls
starts with an ioctl interface where a copy_from_user() is used for
getting user data.
In the entire file (and also others browsed through searching) I do
not see any _user annotations and, sadly, I do not understand
perfectly what you are looking for.

Also, if there is a copy_{to/from}_user call isn't it supposed that
the source, respectively the destination parameter, point to a
userspace pointer?
Or is there any other way this type of functions could be used
successfully with only kernel space pointers?

It would be very appreciated if you could expand on what I should look
for in this situation.

Sorry for the newbie questions.
Thanks a lot,

Ioana

Re: [Outreachy kernel] [PATCH] staging: lustre: lent: add missing __user __user annotation

[Greg KH]

On Fri, Oct 09, 2015 at 12:30:13AM +0300, Ioana Ciornei wrote:
> Add missing __user annotation when using copy_{to/from}_user() to indicate that is
> a pointer from userspace and that it should not be derefenced.
> Also, remove sparse warnings such as:
> 
> drivers/staging/lustre/lnet/selftest/console.c:1474:26: warning: incorrect type in argument 1 (different address spaces)
>   1 drivers/staging/lustre/lnet/selftest/console.c:1474:26:    expected void [noderef] <asn:1>*to
>   2 drivers/staging/lustre/lnet/selftest/console.c:1474:26:    got struct sfw_counters_t [usertype] *[assigned] sfwk_stat
> 
> Signed-off-by: Ioana Ciornei <ciorneiioana@gmail.com>
> ---
>  drivers/staging/lustre/lnet/selftest/console.c | 44 +++++++++++++-------------
>  1 file changed, 22 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/staging/lustre/lnet/selftest/console.c b/drivers/staging/lustre/lnet/selftest/console.c
> index 024aaee..7ba9022 100644
> --- a/drivers/staging/lustre/lnet/selftest/console.c
> +++ b/drivers/staging/lustre/lnet/selftest/console.c
> @@ -384,9 +384,9 @@ lstcon_sesrpc_readent(int transop, srpc_msg_t *msg,
>  	case LST_TRANS_SESQRY:
>  		rep = &msg->msg_body.dbg_reply;
>  
> -		if (copy_to_user(&ent_up->rpe_priv[0],
> +		if (copy_to_user((void __user *)&ent_up->rpe_priv[0],
>  				     &rep->dbg_timeout, sizeof(int)) ||
> -		    copy_to_user(&ent_up->rpe_payload[0],
> +		    copy_to_user((void __user *)&ent_up->rpe_payload[0],
>  				     &rep->dbg_name, LST_NAME_SIZE))
>  			return -EFAULT;
>  

The __user annotation in the lustre code is a mess, lots of work needs
to be done here, but it's complex and tricky, and in the end is going to
probably require some API changes to do correctly.  I strongly recommend
just leaving it alone and letting the lustre developers fix it up
properly sometime in the future, papering over the warnings with these
casts just gives the impression that the issue is fixed, when it really
isn't.

sorry, but I can't take this change.

greg k-h

Re: [Outreachy kernel] [PATCH] staging: lustre: lent: add missing __user __user annotation

[Ioana Ciornei]

[-- Attachment #1: Type: text/plain, Size: 2403 bytes --]

On Oct 9, 2015 6:58 AM, "Greg KH" <gregkh@linuxfoundation.org> wrote:
>
> On Fri, Oct 09, 2015 at 12:30:13AM +0300, Ioana Ciornei wrote:
> > Add missing __user annotation when using copy_{to/from}_user() to
indicate that is
> > a pointer from userspace and that it should not be derefenced.
> > Also, remove sparse warnings such as:
> >
> > drivers/staging/lustre/lnet/selftest/console.c:1474:26: warning:
incorrect type in argument 1 (different address spaces)
> >   1 drivers/staging/lustre/lnet/selftest/console.c:1474:26:    expected
void [noderef] <asn:1>*to
> >   2 drivers/staging/lustre/lnet/selftest/console.c:1474:26:    got
struct sfw_counters_t [usertype] *[assigned] sfwk_stat
> >
> > Signed-off-by: Ioana Ciornei <ciorneiioana@gmail.com>
> > ---
> >  drivers/staging/lustre/lnet/selftest/console.c | 44
+++++++++++++-------------
> >  1 file changed, 22 insertions(+), 22 deletions(-)
> >
> > diff --git a/drivers/staging/lustre/lnet/selftest/console.c
b/drivers/staging/lustre/lnet/selftest/console.c
> > index 024aaee..7ba9022 100644
> > --- a/drivers/staging/lustre/lnet/selftest/console.c
> > +++ b/drivers/staging/lustre/lnet/selftest/console.c
> > @@ -384,9 +384,9 @@ lstcon_sesrpc_readent(int transop, srpc_msg_t *msg,
> >       case LST_TRANS_SESQRY:
> >               rep = &msg->msg_body.dbg_reply;
> >
> > -             if (copy_to_user(&ent_up->rpe_priv[0],
> > +             if (copy_to_user((void __user *)&ent_up->rpe_priv[0],
> >                                    &rep->dbg_timeout, sizeof(int)) ||
> > -                 copy_to_user(&ent_up->rpe_payload[0],
> > +                 copy_to_user((void __user *)&ent_up->rpe_payload[0],
> >                                    &rep->dbg_name, LST_NAME_SIZE))
> >                       return -EFAULT;
> >
>
> The __user annotation in the lustre code is a mess, lots of work needs
> to be done here, but it's complex and tricky, and in the end is going to
> probably require some API changes to do correctly.  I strongly recommend
> just leaving it alone and letting the lustre developers fix it up
> properly sometime in the future, papering over the warnings with these
> casts just gives the impression that the issue is fixed, when it really
> isn't.
>
> sorry, but I can't take this change.

It's ok. Even so, I learnt a lot of new things while reading about
annotations and memory spaces.

Thanks for the feedback,
Ioana

[-- Attachment #2: Type: text/html, Size: 3173 bytes --]

Re: [Outreachy kernel] [PATCH] staging: lustre: lent: add missing __user __user annotation

[Arnd Bergmann]

On Friday 09 October 2015 01:29:59 Ioana Ciornei wrote:
> >> diff --git a/drivers/staging/lustre/lnet/selftest/console.c b/drivers/staging/lustre/lnet/selftest/console.c
> >> index 024aaee..7ba9022 100644
> >> --- a/drivers/staging/lustre/lnet/selftest/console.c
> >> +++ b/drivers/staging/lustre/lnet/selftest/console.c
> >> @@ -384,9 +384,9 @@ lstcon_sesrpc_readent(int transop, srpc_msg_t *msg,
> >>       case LST_TRANS_SESQRY:
> >>               rep = &msg->msg_body.dbg_reply;
> >>
> >> -             if (copy_to_user(&ent_up->rpe_priv[0],
> >> +             if (copy_to_user((void __user *)&ent_up->rpe_priv[0],
> >>                                    &rep->dbg_timeout, sizeof(int)) ||
> >> -                 copy_to_user(&ent_up->rpe_payload[0],
> >> +                 copy_to_user((void __user *)&ent_up->rpe_payload[0],
> >>                                    &rep->dbg_name, LST_NAME_SIZE))
> >>                       return -EFAULT;
> >>
> >
> > This looks wrong. I have not checked what is actually going on here, but
> > you should never add a __user cast to a variable that is accessed
> > through a kernel pointer. Can you find out where the ent_up variable
> > is and why it is not marked __user itself?
> >
> 
> I searched a bit through the source code and the hierarchy of calls
> starts with an ioctl interface where a copy_from_user() is used for
> getting user data.

Ok, good.

> In the entire file (and also others browsed through searching) I do
> not see any _user annotations and, sadly, I do not understand
> perfectly what you are looking for.

The idea of the annotations is to let the compiler know whether a pointer
is for kernel (no annotation), user space (__user), memory mapped I/O (__iomem)
or one of the rarer types, and warn if some code incorrectly assigns
a pointer of one type to another.

The kernel pointers are the only ones that you are allowed to dereference
directly, the others go through special function calls (copy_from_user,
get_user, readl, memcpy_toio, ...) and can only be passed to functions
with the same annotations otherwise.

For an ioctl, you normally get the initial pointer from the ioctl
function argument, and then pass it down through other functions
as a __user * pointer. It sounds like the mistake in this code is that
the annotations are missing completely and need to be added starting
at the point where the 'unsigned long arg' is converted to a pointer.

If a cast is necessary, it should be done at the earliest possible
point, and then the pointers passed down with the same annotation.

I took a closer look at the specific ioctl here and unfortunately I
found that it's done in a rather obscure way by having linked lists
in user space that are walked by the kernel, which can complicate
things a little bit. It would be nice if you can try to add all
the right annotations here, but a better long-term outcome would
be if we could find a way to get rid of these ioctls. I don't know if
they are required for operation of lustre, or if they could be done
differently.

We certainly wouldn't merge code doing this outside of drivers/staging,
but we have made exceptions for ugly APIs in the past when they were
in widespread use.

Greg might be able to comment on the last aspect.

> Also, if there is a copy_{to/from}_user call isn't it supposed that
> the source, respectively the destination parameter, point to a
> userspace pointer?
> Or is there any other way this type of functions could be used
> successfully with only kernel space pointers?

Generally speaking no. If you ever pass a kernel pointer into copy_from_user,
you normally either have a security hole, or a crash, depending on the CPU
architecture.

> It would be very appreciated if you could expand on what I should look
> for in this situation.
> 
> Sorry for the newbie questions.

No worries.

	Arnd