Hi,
I was reading some code and had a question in xenbus_file_write()
drivers/xen/xenbus/xenbus_dev_frontend.c
461 if ((len + u->len) > sizeof(u->u.buffer)) {
^^^^^^^^^^^^
Can this addition overflow? Should the test be something like:
if (len > sizeof(u->u.buffer) || len + u->len >
sizeof(u->u.buffer)) {
462 /* On error, dump existing buffer */
463 u->len = 0;
464 rc = -EINVAL;
465 goto out;
466 }
467
468 ret = copy_from_user(u->u.buffer + u->len, ubuf, len);
469
regards,
dan carpenter
On Thu, 2012-09-13 at 19:00 +0300, Dan Carpenter wrote:> Hi,Thanks Dan. I'm not sure anyone from Xen-land really monitors virtualization at . Adding xen-devel and Konrad.> > I was reading some code and had a question in xenbus_file_write() > > drivers/xen/xenbus/xenbus_dev_frontend.c > 461 if ((len + u->len) > sizeof(u->u.buffer)) { > ^^^^^^^^^^^^ > Can this addition overflow?len is a size_t and u->len is an unsigned int, so I expect so.> Should the test be something like: > > if (len > sizeof(u->u.buffer) || len + u->len > sizeof(u->u.buffer)) {I think that would do it. Ian.> 462 /* On error, dump existing buffer */ > 463 u->len = 0; > 464 rc = -EINVAL; > 465 goto out; > 466 } > 467 > 468 ret = copy_from_user(u->u.buffer + u->len, ubuf, len); > 469 > > regards, > dan carpenter > _______________________________________________ > Virtualization mailing list > Virtualization at lists.linux-foundation.org > https://lists.linuxfoundation.org/mailman/listinfo/virtualization >
>>> On 15.10.12 at 12:25, Ian Campbell <Ian.Campbell@citrix.com> wrote: > On Thu, 2012-09-13 at 19:00 +0300, Dan Carpenter wrote: >> Hi, > > Thanks Dan. I''m not sure anyone from Xen-land really monitors > virtualization@. Adding xen-devel and Konrad. > >> >> I was reading some code and had a question in xenbus_file_write() >> >> drivers/xen/xenbus/xenbus_dev_frontend.c >> 461 if ((len + u->len) > sizeof(u->u.buffer)) { >> ^^^^^^^^^^^^ >> Can this addition overflow? > > len is a size_t and u->len is an unsigned int, so I expect so. > >> Should the test be something like: >> >> if (len > sizeof(u->u.buffer) || len + u->len > sizeof(u->u.buffer)) { > > I think that would do it.Actually, it can remain a single range check: if (len > sizeof(u->u.buffer) - u->len) { because the rest of the code guarantees u->len to be less or equal to sizeof(u->u.buffer) at all times. Jan