* [patch 2/2 v2] staging: lustre: integer overflow in obd_ioctl_is_invalid() [not found] <20140425072317.GD4963@mwanda> @ 2014-04-28 10:58 ` Dan Carpenter 2014-04-28 11:35 ` walter harms 0 siblings, 1 reply; 3+ 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] 3+ 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] staging: lustre: integer overflow in obd_ioctl_is_invalid() Dan Carpenter @ 2014-04-28 11:35 ` walter harms 2014-04-28 11:41 ` Dan Carpenter 0 siblings, 1 reply; 3+ 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] 3+ 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; 3+ 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] 3+ messages in thread
end of thread, other threads:[~2014-04-28 11:41 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20140425072317.GD4963@mwanda>
2014-04-28 10:58 ` [patch 2/2 v2] staging: lustre: integer overflow in obd_ioctl_is_invalid() 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