potential integer overflow in xenbus_file_write()

potential integer overflow in xenbus_file_write()

[Dan Carpenter]

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

Re: potential integer overflow in xenbus_file_write()

[Ian Campbell]

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.

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@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/virtualization
> 

Re: [Xen-devel] potential integer overflow in xenbus_file_write()

[Jan Beulich]

>>> 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