* [patch 2/2] staging: lustre: integer overflow in obd_ioctl_is_invalid()
@ 2014-04-24 21:49 Dan Carpenter
2014-04-25 3:22 ` Peng Tao
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Dan Carpenter @ 2014-04-24 21:49 UTC (permalink / raw)
To: kernel-janitors
The obd_ioctl_getdata() function caps "data->ioc_len" at
OBD_MAX_IOCTL_BUFFER and then calls this obd_ioctl_is_invalid() to check
that the other values inside data are valid.
There are several lengths inside data but when they are added together
they must not be larger than "data->ioc_len". The checks against
"(data->ioc_inllen1 > (1<<30))" are supposed to ensure that the addition
does not have an integer overflow. But "(1<<30) * 4" actually can
overflow 32 bits so the checks are insufficient.
I have changed it to "> OBD_MAX_IOCTL_BUFFER" instead.
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
diff --git a/drivers/staging/lustre/lustre/include/lustre_lib.h b/drivers/staging/lustre/lustre/include/lustre_lib.h
index 0368ca6..04f549e 100644
--- a/drivers/staging/lustre/lustre/include/lustre_lib.h
+++ b/drivers/staging/lustre/lustre/include/lustre_lib.h
@@ -192,23 +192,23 @@ static inline int obd_ioctl_packlen(struct obd_ioctl_data *data)
static inline int obd_ioctl_is_invalid(struct obd_ioctl_data *data)
{
- if (data->ioc_len > (1<<30)) {
+ if (data->ioc_len > OBD_MAX_IOCTL_BUFFER) {
CERROR("OBD ioctl: ioc_len larger than 1<<30\n");
return 1;
}
- if (data->ioc_inllen1 > (1<<30)) {
+ if (data->ioc_inllen1 > OBD_MAX_IOCTL_BUFFER) {
CERROR("OBD ioctl: ioc_inllen1 larger than 1<<30\n");
return 1;
}
- if (data->ioc_inllen2 > (1<<30)) {
+ if (data->ioc_inllen2 > OBD_MAX_IOCTL_BUFFER) {
CERROR("OBD ioctl: ioc_inllen2 larger than 1<<30\n");
return 1;
}
- if (data->ioc_inllen3 > (1<<30)) {
+ if (data->ioc_inllen3 > OBD_MAX_IOCTL_BUFFER) {
CERROR("OBD ioctl: ioc_inllen3 larger than 1<<30\n");
return 1;
}
- if (data->ioc_inllen4 > (1<<30)) {
+ if (data->ioc_inllen4 > OBD_MAX_IOCTL_BUFFER) {
CERROR("OBD ioctl: ioc_inllen4 larger than 1<<30\n");
return 1;
}
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [patch 2/2] staging: lustre: integer overflow in obd_ioctl_is_invalid() 2014-04-24 21:49 [patch 2/2] staging: lustre: integer overflow in obd_ioctl_is_invalid() Dan Carpenter @ 2014-04-25 3:22 ` Peng Tao 2014-04-25 7:13 ` walter harms 2014-04-25 7:23 ` Dan Carpenter 2 siblings, 0 replies; 7+ messages in thread From: Peng Tao @ 2014-04-25 3:22 UTC (permalink / raw) To: kernel-janitors On Fri, Apr 25, 2014 at 5:49 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote: > The obd_ioctl_getdata() function caps "data->ioc_len" at > OBD_MAX_IOCTL_BUFFER and then calls this obd_ioctl_is_invalid() to check > that the other values inside data are valid. > > There are several lengths inside data but when they are added together > they must not be larger than "data->ioc_len". The checks against > "(data->ioc_inllen1 > (1<<30))" are supposed to ensure that the addition > does not have an integer overflow. But "(1<<30) * 4" actually can > overflow 32 bits so the checks are insufficient. > > I have changed it to "> OBD_MAX_IOCTL_BUFFER" instead. > Reviewed-by: Peng Tao <bergwolf@gmail.com> Thanks, Tao > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > diff --git a/drivers/staging/lustre/lustre/include/lustre_lib.h b/drivers/staging/lustre/lustre/include/lustre_lib.h > index 0368ca6..04f549e 100644 > --- a/drivers/staging/lustre/lustre/include/lustre_lib.h > +++ b/drivers/staging/lustre/lustre/include/lustre_lib.h > @@ -192,23 +192,23 @@ static inline int obd_ioctl_packlen(struct obd_ioctl_data *data) > > static inline int obd_ioctl_is_invalid(struct obd_ioctl_data *data) > { > - if (data->ioc_len > (1<<30)) { > + if (data->ioc_len > OBD_MAX_IOCTL_BUFFER) { > CERROR("OBD ioctl: ioc_len larger than 1<<30\n"); > return 1; > } > - if (data->ioc_inllen1 > (1<<30)) { > + if (data->ioc_inllen1 > OBD_MAX_IOCTL_BUFFER) { > CERROR("OBD ioctl: ioc_inllen1 larger than 1<<30\n"); > return 1; > } > - if (data->ioc_inllen2 > (1<<30)) { > + if (data->ioc_inllen2 > OBD_MAX_IOCTL_BUFFER) { > CERROR("OBD ioctl: ioc_inllen2 larger than 1<<30\n"); > return 1; > } > - if (data->ioc_inllen3 > (1<<30)) { > + if (data->ioc_inllen3 > OBD_MAX_IOCTL_BUFFER) { > CERROR("OBD ioctl: ioc_inllen3 larger than 1<<30\n"); > return 1; > } > - if (data->ioc_inllen4 > (1<<30)) { > + if (data->ioc_inllen4 > OBD_MAX_IOCTL_BUFFER) { > CERROR("OBD ioctl: ioc_inllen4 larger than 1<<30\n"); > return 1; > } ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [patch 2/2] staging: lustre: integer overflow in obd_ioctl_is_invalid() 2014-04-24 21:49 [patch 2/2] staging: lustre: integer overflow in obd_ioctl_is_invalid() Dan Carpenter 2014-04-25 3:22 ` Peng Tao @ 2014-04-25 7:13 ` walter harms 2014-04-25 7:23 ` Dan Carpenter 2 siblings, 0 replies; 7+ messages in thread From: walter harms @ 2014-04-25 7:13 UTC (permalink / raw) To: kernel-janitors Am 24.04.2014 23:49, schrieb Dan Carpenter: > The obd_ioctl_getdata() function caps "data->ioc_len" at > OBD_MAX_IOCTL_BUFFER and then calls this obd_ioctl_is_invalid() to check > that the other values inside data are valid. > > There are several lengths inside data but when they are added together > they must not be larger than "data->ioc_len". The checks against > "(data->ioc_inllen1 > (1<<30))" are supposed to ensure that the addition > does not have an integer overflow. But "(1<<30) * 4" actually can > overflow 32 bits so the checks are insufficient. > > I have changed it to "> OBD_MAX_IOCTL_BUFFER" instead. > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > diff --git a/drivers/staging/lustre/lustre/include/lustre_lib.h b/drivers/staging/lustre/lustre/include/lustre_lib.h > index 0368ca6..04f549e 100644 > --- a/drivers/staging/lustre/lustre/include/lustre_lib.h > +++ b/drivers/staging/lustre/lustre/include/lustre_lib.h > @@ -192,23 +192,23 @@ static inline int obd_ioctl_packlen(struct obd_ioctl_data *data) > > static inline int obd_ioctl_is_invalid(struct obd_ioctl_data *data) > { > - if (data->ioc_len > (1<<30)) { > + if (data->ioc_len > OBD_MAX_IOCTL_BUFFER) { > CERROR("OBD ioctl: ioc_len larger than 1<<30\n"); > return 1; > } I would suggest to adjust the errormsg also like: CERROR("OBD ioctl: ioc_len larger than OBD_MAX_IOCTL_BUFFER\n"); otherwise future debuggers will be confused. just my 2 cents re, wh > - if (data->ioc_inllen1 > (1<<30)) { > + if (data->ioc_inllen1 > OBD_MAX_IOCTL_BUFFER) { > CERROR("OBD ioctl: ioc_inllen1 larger than 1<<30\n"); > return 1; > } > - if (data->ioc_inllen2 > (1<<30)) { > + if (data->ioc_inllen2 > OBD_MAX_IOCTL_BUFFER) { > CERROR("OBD ioctl: ioc_inllen2 larger than 1<<30\n"); > return 1; > } > - if (data->ioc_inllen3 > (1<<30)) { > + if (data->ioc_inllen3 > OBD_MAX_IOCTL_BUFFER) { > CERROR("OBD ioctl: ioc_inllen3 larger than 1<<30\n"); > return 1; > } > - if (data->ioc_inllen4 > (1<<30)) { > + if (data->ioc_inllen4 > OBD_MAX_IOCTL_BUFFER) { > CERROR("OBD ioctl: ioc_inllen4 larger than 1<<30\n"); > return 1; > } > -- > To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [patch 2/2] staging: lustre: integer overflow in obd_ioctl_is_invalid() 2014-04-24 21:49 [patch 2/2] staging: lustre: integer overflow in obd_ioctl_is_invalid() Dan Carpenter 2014-04-25 3:22 ` Peng Tao 2014-04-25 7:13 ` walter harms @ 2014-04-25 7:23 ` Dan Carpenter 2014-04-28 10:58 ` [patch 2/2 v2] " Dan Carpenter 2 siblings, 1 reply; 7+ messages in thread From: Dan Carpenter @ 2014-04-25 7:23 UTC (permalink / raw) To: kernel-janitors On Fri, Apr 25, 2014 at 09:13:21AM +0200, walter harms wrote: > > > Am 24.04.2014 23:49, schrieb Dan Carpenter: > > The obd_ioctl_getdata() function caps "data->ioc_len" at > > OBD_MAX_IOCTL_BUFFER and then calls this obd_ioctl_is_invalid() to check > > that the other values inside data are valid. > > > > There are several lengths inside data but when they are added together > > they must not be larger than "data->ioc_len". The checks against > > "(data->ioc_inllen1 > (1<<30))" are supposed to ensure that the addition > > does not have an integer overflow. But "(1<<30) * 4" actually can > > overflow 32 bits so the checks are insufficient. > > > > I have changed it to "> OBD_MAX_IOCTL_BUFFER" instead. > > > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > > > diff --git a/drivers/staging/lustre/lustre/include/lustre_lib.h b/drivers/staging/lustre/lustre/include/lustre_lib.h > > index 0368ca6..04f549e 100644 > > --- a/drivers/staging/lustre/lustre/include/lustre_lib.h > > +++ b/drivers/staging/lustre/lustre/include/lustre_lib.h > > @@ -192,23 +192,23 @@ static inline int obd_ioctl_packlen(struct obd_ioctl_data *data) > > > > static inline int obd_ioctl_is_invalid(struct obd_ioctl_data *data) > > { > > - if (data->ioc_len > (1<<30)) { > > + if (data->ioc_len > OBD_MAX_IOCTL_BUFFER) { > > CERROR("OBD ioctl: ioc_len larger than 1<<30\n"); > > return 1; > > } > > I would suggest to adjust the errormsg also like: > CERROR("OBD ioctl: ioc_len larger than OBD_MAX_IOCTL_BUFFER\n"); > > otherwise future debuggers will be confused. > > just my 2 cents > Ah yes. I will resend. regards, dan carpenter ^ permalink raw reply [flat|nested] 7+ messages in thread
* [patch 2/2 v2] staging: lustre: integer overflow in obd_ioctl_is_invalid() 2014-04-25 7:23 ` Dan Carpenter @ 2014-04-28 10:58 ` Dan Carpenter 2014-04-28 11:35 ` walter harms 0 siblings, 1 reply; 7+ messages in thread From: Dan Carpenter @ 2014-04-28 10:58 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Peng Tao, Andreas Dilger, Oleg Drokin, John L. Hammond, Dmitry Eremin, Jinshan Xiong, Dulshani Gunawardhana, devel, linux-kernel, kernel-janitors, walter harms The obd_ioctl_getdata() function caps "data->ioc_len" at OBD_MAX_IOCTL_BUFFER and then calls this obd_ioctl_is_invalid() to check that the other values inside data are valid. There are several lengths inside data but when they are added together they must not be larger than "data->ioc_len". The checks against "(data->ioc_inllen1 > (1<<30))" are supposed to ensure that the addition does not have an integer overflow. But "(1<<30) * 4" actually can overflow 32 bits, so the checks are insufficient. I have changed it to "> OBD_MAX_IOCTL_BUFFER" instead. Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> --- v2: Updated the error messages as Walter Harms pointed out. diff --git a/drivers/staging/lustre/lustre/include/lustre_lib.h b/drivers/staging/lustre/lustre/include/lustre_lib.h index bdc9812..3c26bbd 100644 --- a/drivers/staging/lustre/lustre/include/lustre_lib.h +++ b/drivers/staging/lustre/lustre/include/lustre_lib.h @@ -179,24 +179,25 @@ static inline int obd_ioctl_packlen(struct obd_ioctl_data *data) static inline int obd_ioctl_is_invalid(struct obd_ioctl_data *data) { - if (data->ioc_len > (1<<30)) { - CERROR("OBD ioctl: ioc_len larger than 1<<30\n"); + if (data->ioc_len > OBD_MAX_IOCTL_BUFFER) { + CERROR("OBD ioctl: ioc_len larger than %d\n", + OBD_MAX_IOCTL_BUFFER); return 1; } - if (data->ioc_inllen1 > (1<<30)) { - CERROR("OBD ioctl: ioc_inllen1 larger than 1<<30\n"); + if (data->ioc_inllen1 > OBD_MAX_IOCTL_BUFFER) { + CERROR("OBD ioctl: ioc_inllen1 larger than ioc_len\n"); return 1; } - if (data->ioc_inllen2 > (1<<30)) { - CERROR("OBD ioctl: ioc_inllen2 larger than 1<<30\n"); + if (data->ioc_inllen2 > OBD_MAX_IOCTL_BUFFER) { + CERROR("OBD ioctl: ioc_inllen2 larger than ioc_len\n"); return 1; } - if (data->ioc_inllen3 > (1<<30)) { - CERROR("OBD ioctl: ioc_inllen3 larger than 1<<30\n"); + if (data->ioc_inllen3 > OBD_MAX_IOCTL_BUFFER) { + CERROR("OBD ioctl: ioc_inllen3 larger than ioc_len\n"); return 1; } - if (data->ioc_inllen4 > (1<<30)) { - CERROR("OBD ioctl: ioc_inllen4 larger than 1<<30\n"); + if (data->ioc_inllen4 > OBD_MAX_IOCTL_BUFFER) { + CERROR("OBD ioctl: ioc_inllen4 larger than ioc_len\n"); return 1; } if (data->ioc_inlbuf1 && !data->ioc_inllen1) { ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [patch 2/2 v2] staging: lustre: integer overflow in obd_ioctl_is_invalid() 2014-04-28 10:58 ` [patch 2/2 v2] " Dan Carpenter @ 2014-04-28 11:35 ` walter harms 2014-04-28 11:41 ` Dan Carpenter 0 siblings, 1 reply; 7+ messages in thread From: walter harms @ 2014-04-28 11:35 UTC (permalink / raw) To: Dan Carpenter Cc: Greg Kroah-Hartman, Peng Tao, Andreas Dilger, Oleg Drokin, John L. Hammond, Dmitry Eremin, Jinshan Xiong, Dulshani Gunawardhana, devel, linux-kernel, kernel-janitors Am 28.04.2014 12:58, schrieb Dan Carpenter: > The obd_ioctl_getdata() function caps "data->ioc_len" at > OBD_MAX_IOCTL_BUFFER and then calls this obd_ioctl_is_invalid() to check > that the other values inside data are valid. > > There are several lengths inside data but when they are added together > they must not be larger than "data->ioc_len". The checks against > "(data->ioc_inllen1 > (1<<30))" are supposed to ensure that the addition > does not have an integer overflow. But "(1<<30) * 4" actually can > overflow 32 bits, so the checks are insufficient. > > I have changed it to "> OBD_MAX_IOCTL_BUFFER" instead. > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > --- > v2: Updated the error messages as Walter Harms pointed out. > > diff --git a/drivers/staging/lustre/lustre/include/lustre_lib.h b/drivers/staging/lustre/lustre/include/lustre_lib.h > index bdc9812..3c26bbd 100644 > --- a/drivers/staging/lustre/lustre/include/lustre_lib.h > +++ b/drivers/staging/lustre/lustre/include/lustre_lib.h > @@ -179,24 +179,25 @@ static inline int obd_ioctl_packlen(struct obd_ioctl_data *data) > > static inline int obd_ioctl_is_invalid(struct obd_ioctl_data *data) > { > - if (data->ioc_len > (1<<30)) { > - CERROR("OBD ioctl: ioc_len larger than 1<<30\n"); > + if (data->ioc_len > OBD_MAX_IOCTL_BUFFER) { > + CERROR("OBD ioctl: ioc_len larger than %d\n", > + OBD_MAX_IOCTL_BUFFER); > return 1; > } > - if (data->ioc_inllen1 > (1<<30)) { > - CERROR("OBD ioctl: ioc_inllen1 larger than 1<<30\n"); > + if (data->ioc_inllen1 > OBD_MAX_IOCTL_BUFFER) { > + CERROR("OBD ioctl: ioc_inllen1 larger than ioc_len\n"); > return 1; > } The error mention ioc_len the compare is OBD_MAX_IOCTL_BUFFER ? Is that intentional ? re, wh > - if (data->ioc_inllen2 > (1<<30)) { > - CERROR("OBD ioctl: ioc_inllen2 larger than 1<<30\n"); > + if (data->ioc_inllen2 > OBD_MAX_IOCTL_BUFFER) { > + CERROR("OBD ioctl: ioc_inllen2 larger than ioc_len\n"); > return 1; > } > - if (data->ioc_inllen3 > (1<<30)) { > - CERROR("OBD ioctl: ioc_inllen3 larger than 1<<30\n"); > + if (data->ioc_inllen3 > OBD_MAX_IOCTL_BUFFER) { > + CERROR("OBD ioctl: ioc_inllen3 larger than ioc_len\n"); > return 1; > } > - if (data->ioc_inllen4 > (1<<30)) { > - CERROR("OBD ioctl: ioc_inllen4 larger than 1<<30\n"); > + if (data->ioc_inllen4 > OBD_MAX_IOCTL_BUFFER) { > + CERROR("OBD ioctl: ioc_inllen4 larger than ioc_len\n"); > return 1; > } > if (data->ioc_inlbuf1 && !data->ioc_inllen1) { > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [patch 2/2 v2] staging: lustre: integer overflow in obd_ioctl_is_invalid() 2014-04-28 11:35 ` walter harms @ 2014-04-28 11:41 ` Dan Carpenter 0 siblings, 0 replies; 7+ messages in thread From: Dan Carpenter @ 2014-04-28 11:41 UTC (permalink / raw) To: walter harms Cc: Greg Kroah-Hartman, Peng Tao, Andreas Dilger, Oleg Drokin, John L. Hammond, Dmitry Eremin, Jinshan Xiong, Dulshani Gunawardhana, devel, linux-kernel, kernel-janitors On Mon, Apr 28, 2014 at 01:35:19PM +0200, walter harms wrote: > > > Am 28.04.2014 12:58, schrieb Dan Carpenter: > > The obd_ioctl_getdata() function caps "data->ioc_len" at > > OBD_MAX_IOCTL_BUFFER and then calls this obd_ioctl_is_invalid() to check > > that the other values inside data are valid. > > > > There are several lengths inside data but when they are added together > > they must not be larger than "data->ioc_len". The checks against > > "(data->ioc_inllen1 > (1<<30))" are supposed to ensure that the addition > > does not have an integer overflow. But "(1<<30) * 4" actually can > > overflow 32 bits, so the checks are insufficient. > > > > I have changed it to "> OBD_MAX_IOCTL_BUFFER" instead. > > > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > --- > > v2: Updated the error messages as Walter Harms pointed out. > > > > diff --git a/drivers/staging/lustre/lustre/include/lustre_lib.h b/drivers/staging/lustre/lustre/include/lustre_lib.h > > index bdc9812..3c26bbd 100644 > > --- a/drivers/staging/lustre/lustre/include/lustre_lib.h > > +++ b/drivers/staging/lustre/lustre/include/lustre_lib.h > > @@ -179,24 +179,25 @@ static inline int obd_ioctl_packlen(struct obd_ioctl_data *data) > > > > static inline int obd_ioctl_is_invalid(struct obd_ioctl_data *data) > > { > > - if (data->ioc_len > (1<<30)) { > > - CERROR("OBD ioctl: ioc_len larger than 1<<30\n"); > > + if (data->ioc_len > OBD_MAX_IOCTL_BUFFER) { > > + CERROR("OBD ioctl: ioc_len larger than %d\n", > > + OBD_MAX_IOCTL_BUFFER); > > return 1; > > } > > - if (data->ioc_inllen1 > (1<<30)) { > > - CERROR("OBD ioctl: ioc_inllen1 larger than 1<<30\n"); > > + if (data->ioc_inllen1 > OBD_MAX_IOCTL_BUFFER) { > > + CERROR("OBD ioctl: ioc_inllen1 larger than ioc_len\n"); > > return 1; > > } > > The error mention ioc_len the compare is OBD_MAX_IOCTL_BUFFER ? > Is that intentional ? Yep. regards, dan carpenter ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2014-04-28 11:41 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-04-24 21:49 [patch 2/2] staging: lustre: integer overflow in obd_ioctl_is_invalid() Dan Carpenter 2014-04-25 3:22 ` Peng Tao 2014-04-25 7:13 ` walter harms 2014-04-25 7:23 ` Dan Carpenter 2014-04-28 10:58 ` [patch 2/2 v2] " Dan Carpenter 2014-04-28 11:35 ` walter harms 2014-04-28 11:41 ` Dan Carpenter
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox