* [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).