kernel-janitors.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [patch] staging: lustre: potential underflow in mdc_iocontrol()
@ 2015-01-06  9:57 Dan Carpenter
  2015-10-29 13:51 ` [patch] staging: lustre: potential underflow in libcfs_kkuc_group_add() Dan Carpenter
  0 siblings, 1 reply; 6+ messages in thread
From: Dan Carpenter @ 2015-01-06  9:57 UTC (permalink / raw)
  To: kernel-janitors

Smatch complains that "data->ioc_plen2" is a user controlled value and,
since we cast to signed int, the limit check can underflow.  It's not
very serious because probably the copy_to_user() would return -EFAULT
on every arch that matters instead of creating an info leak.  Also I
haven't followed it through to see if the value is really user
controlled.

But definitely it would be safer to cast to unsigned so let's do that.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

diff --git a/drivers/staging/lustre/lustre/mdc/mdc_request.c b/drivers/staging/lustre/lustre/mdc/mdc_request.c
index 3b0f245..05d05ce 100644
--- a/drivers/staging/lustre/lustre/mdc/mdc_request.c
+++ b/drivers/staging/lustre/lustre/mdc/mdc_request.c
@@ -1908,8 +1908,8 @@ static int mdc_iocontrol(unsigned int cmd, struct obd_export *exp, int len,
 
 		/* copy UUID */
 		if (copy_to_user(data->ioc_pbuf2, obd2cli_tgt(obd),
-				     min((int) data->ioc_plen2,
-					 (int) sizeof(struct obd_uuid)))) {
+				 min_t(size_t, data->ioc_plen2,
+					       sizeof(struct obd_uuid)))) {
 			rc = -EFAULT;
 			goto out;
 		}
@@ -1921,8 +1921,8 @@ static int mdc_iocontrol(unsigned int cmd, struct obd_export *exp, int len,
 			goto out;
 
 		if (copy_to_user(data->ioc_pbuf1, &stat_buf,
-				     min((int) data->ioc_plen1,
-					 (int) sizeof(stat_buf)))) {
+				 min_t(size_t, data->ioc_plen1,
+					       sizeof(stat_buf)))) {
 			rc = -EFAULT;
 			goto out;
 		}

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [patch] staging: lustre: potential underflow in libcfs_kkuc_group_add()
  2015-01-06  9:57 [patch] staging: lustre: potential underflow in mdc_iocontrol() Dan Carpenter
@ 2015-10-29 13:51 ` Dan Carpenter
  2015-11-03 23:28   ` Simmons, James A.
  0 siblings, 1 reply; 6+ messages in thread
From: Dan Carpenter @ 2015-10-29 13:51 UTC (permalink / raw)
  To: lustre-devel

My static checker says that "group" is a user controlled number that can
be negative leading to an array underflow.  I have looked at it, and I'm
not an expert enough in lustre to say for sure if it is really a bug.
Anyway, it's simple enough to make the variable unsigned which pleases
the static checker and makes it easier to audit.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

diff --git a/drivers/staging/lustre/include/linux/libcfs/libcfs_kernelcomm.h b/drivers/staging/lustre/include/linux/libcfs/libcfs_kernelcomm.h
index a989d26..41f3d81 100644
--- a/drivers/staging/lustre/include/linux/libcfs/libcfs_kernelcomm.h
+++ b/drivers/staging/lustre/include/linux/libcfs/libcfs_kernelcomm.h
@@ -91,7 +91,7 @@ typedef int (*libcfs_kkuc_cb_t)(__u32 data, void *cb_arg);
 /* Kernel methods */
 int libcfs_kkuc_msg_put(struct file *fp, void *payload);
 int libcfs_kkuc_group_put(int group, void *payload);
-int libcfs_kkuc_group_add(struct file *fp, int uid, int group,
+int libcfs_kkuc_group_add(struct file *fp, int uid, unsigned int group,
 				 __u32 data);
 int libcfs_kkuc_group_rem(int uid, int group);
 int libcfs_kkuc_group_foreach(int group, libcfs_kkuc_cb_t cb_func,
diff --git a/drivers/staging/lustre/lustre/libcfs/kernel_user_comm.c b/drivers/staging/lustre/lustre/libcfs/kernel_user_comm.c
index ad661a3..d8230ae 100644
--- a/drivers/staging/lustre/lustre/libcfs/kernel_user_comm.c
+++ b/drivers/staging/lustre/lustre/libcfs/kernel_user_comm.c
@@ -110,7 +110,8 @@ static DECLARE_RWSEM(kg_sem);
  * @param uid identifier for this receiver
  * @param group group number
  */
-int libcfs_kkuc_group_add(struct file *filp, int uid, int group, __u32 data)
+int libcfs_kkuc_group_add(struct file *filp, int uid, unsigned int group,
+			  __u32 data)
 {
 	struct kkuc_reg *reg;
 

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* RE: [patch] staging: lustre: potential underflow in libcfs_kkuc_group_add()
  2015-10-29 13:51 ` [patch] staging: lustre: potential underflow in libcfs_kkuc_group_add() Dan Carpenter
@ 2015-11-03 23:28   ` Simmons, James A.
  2015-11-03 23:49     ` Frank Zago
  0 siblings, 1 reply; 6+ messages in thread
From: Simmons, James A. @ 2015-11-03 23:28 UTC (permalink / raw)
  To: lustre-devel


>My static checker says that "group" is a user controlled number that can
>be negative leading to an array underflow.  I have looked at it, and I'm
>not an expert enough in lustre to say for sure if it is really a bug.
>Anyway, it's simple enough to make the variable unsigned which pleases
>the static checker and makes it easier to audit.
>
>Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

Dmitry do you this this could be placed under place under LU-6303 or does a new ticket need to be open.
I also CC Frank Zago who is very familiar with this code. To me it looks okay.

diff --git a/drivers/staging/lustre/include/linux/libcfs/libcfs_kernelcomm.h b/drivers/staging/lustre/include/linux/libcfs/libcfs_kernelcomm.h
index a989d26..41f3d81 100644
--- a/drivers/staging/lustre/include/linux/libcfs/libcfs_kernelcomm.h
+++ b/drivers/staging/lustre/include/linux/libcfs/libcfs_kernelcomm.h
@@ -91,7 +91,7 @@ typedef int (*libcfs_kkuc_cb_t)(__u32 data, void *cb_arg);
 /* Kernel methods */
 int libcfs_kkuc_msg_put(struct file *fp, void *payload);
 int libcfs_kkuc_group_put(int group, void *payload);
-int libcfs_kkuc_group_add(struct file *fp, int uid, int group,
+int libcfs_kkuc_group_add(struct file *fp, int uid, unsigned int group,
 				 __u32 data);
 int libcfs_kkuc_group_rem(int uid, int group);
 int libcfs_kkuc_group_foreach(int group, libcfs_kkuc_cb_t cb_func,
diff --git a/drivers/staging/lustre/lustre/libcfs/kernel_user_comm.c b/drivers/staging/lustre/lustre/libcfs/kernel_user_comm.c
index ad661a3..d8230ae 100644
--- a/drivers/staging/lustre/lustre/libcfs/kernel_user_comm.c
+++ b/drivers/staging/lustre/lustre/libcfs/kernel_user_comm.c
@@ -110,7 +110,8 @@ static DECLARE_RWSEM(kg_sem);
  * @param uid identifier for this receiver
  * @param group group number
  */
-int libcfs_kkuc_group_add(struct file *filp, int uid, int group, __u32 data)
+int libcfs_kkuc_group_add(struct file *filp, int uid, unsigned int group,
+			  __u32 data)
 {
 	struct kkuc_reg *reg;
 
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [patch] staging: lustre: potential underflow in libcfs_kkuc_group_add()
  2015-11-03 23:28   ` Simmons, James A.
@ 2015-11-03 23:49     ` Frank Zago
  2015-11-04 18:25       ` Dan Carpenter
  0 siblings, 1 reply; 6+ messages in thread
From: Frank Zago @ 2015-11-03 23:49 UTC (permalink / raw)
  To: lustre-devel

On 11/03/2015 05:28 PM, Simmons, James A. wrote:
>
>> My static checker says that "group" is a user controlled number that can
>> be negative leading to an array underflow.  I have looked at it, and I'm
>> not an expert enough in lustre to say for sure if it is really a bug.
>> Anyway, it's simple enough to make the variable unsigned which pleases
>> the static checker and makes it easier to audit.
>>
>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>
> Dmitry do you this this could be placed under place under LU-6303 or does a new ticket need to be open.
> I also CC Frank Zago who is very familiar with this code. To me it looks okay.
>
> diff --git a/drivers/staging/lustre/include/linux/libcfs/libcfs_kernelcomm.h b/drivers/staging/lustre/include/linux/libcfs/libcfs_kernelcomm.h
> index a989d26..41f3d81 100644
> --- a/drivers/staging/lustre/include/linux/libcfs/libcfs_kernelcomm.h
> +++ b/drivers/staging/lustre/include/linux/libcfs/libcfs_kernelcomm.h
> @@ -91,7 +91,7 @@ typedef int (*libcfs_kkuc_cb_t)(__u32 data, void *cb_arg);
>   /* Kernel methods */
>   int libcfs_kkuc_msg_put(struct file *fp, void *payload);
>   int libcfs_kkuc_group_put(int group, void *payload);
> -int libcfs_kkuc_group_add(struct file *fp, int uid, int group,
> +int libcfs_kkuc_group_add(struct file *fp, int uid, unsigned int group,
>   				 __u32 data);
>   int libcfs_kkuc_group_rem(int uid, int group);
>   int libcfs_kkuc_group_foreach(int group, libcfs_kkuc_cb_t cb_func,
> diff --git a/drivers/staging/lustre/lustre/libcfs/kernel_user_comm.c b/drivers/staging/lustre/lustre/libcfs/kernel_user_comm.c
> index ad661a3..d8230ae 100644
> --- a/drivers/staging/lustre/lustre/libcfs/kernel_user_comm.c
> +++ b/drivers/staging/lustre/lustre/libcfs/kernel_user_comm.c
> @@ -110,7 +110,8 @@ static DECLARE_RWSEM(kg_sem);
>    * @param uid identifier for this receiver
>    * @param group group number
>    */
> -int libcfs_kkuc_group_add(struct file *filp, int uid, int group, __u32 data)
> +int libcfs_kkuc_group_add(struct file *filp, int uid, unsigned int group,
> +			  __u32 data)
>   {
>   	struct kkuc_reg *reg;

Yes, but for consistency, all 4 functions should be changed.

Regards,
   Frank.


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [patch] staging: lustre: potential underflow in libcfs_kkuc_group_add()
  2015-11-03 23:49     ` Frank Zago
@ 2015-11-04 18:25       ` Dan Carpenter
  2015-11-04 19:00         ` Simmons, James A.
  0 siblings, 1 reply; 6+ messages in thread
From: Dan Carpenter @ 2015-11-04 18:25 UTC (permalink / raw)
  To: lustre-devel

On Tue, Nov 03, 2015 at 05:49:00PM -0600, Frank Zago wrote:
> Yes, but for consistency, all 4 functions should be changed.

Thanks for the review.  I will send a v2.

regards,
dan carpenter


^ permalink raw reply	[flat|nested] 6+ messages in thread

* RE: [patch] staging: lustre: potential underflow in libcfs_kkuc_group_add()
  2015-11-04 18:25       ` Dan Carpenter
@ 2015-11-04 19:00         ` Simmons, James A.
  0 siblings, 0 replies; 6+ messages in thread
From: Simmons, James A. @ 2015-11-04 19:00 UTC (permalink / raw)
  To: lustre-devel

>On Tue, Nov 03, 2015 at 05:49:00PM -0600, Frank Zago wrote:
>> Yes, but for consistency, all 4 functions should be changed.
>
>Thanks for the review.  I will send a v2.

Greg merged your original patch so it will be a new patch.


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2015-11-04 19:00 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-01-06  9:57 [patch] staging: lustre: potential underflow in mdc_iocontrol() Dan Carpenter
2015-10-29 13:51 ` [patch] staging: lustre: potential underflow in libcfs_kkuc_group_add() Dan Carpenter
2015-11-03 23:28   ` Simmons, James A.
2015-11-03 23:49     ` Frank Zago
2015-11-04 18:25       ` Dan Carpenter
2015-11-04 19:00         ` Simmons, James A.

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).