linux-kernel - Re: Intel MIC host driver: possible signed underflow (undefined behavior) in userspace API

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Fri, 10 Jan 2014 06:21:16 -0800
From:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To:	Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
Cc:	Ashutosh Dixit <ashutosh.dixit@...el.com>,
	Sudeep Dutt <sudeep.dutt@...el.com>,
	Caz Yokoyama <Caz.Yokoyama@...el.com>,
	Dasaratharaman Chandramouli 
	<dasaratharaman.chandramouli@...el.com>,
	Nikhil Rao <nikhil.rao@...el.com>,
	Harshavardhan R Kharche <harshavardhan.r.kharche@...el.com>,
	Peter P Waskiewicz Jr <peter.p.waskiewicz.jr@...el.com>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: Intel MIC host driver: possible signed underflow (undefined
 behavior) in userspace API

On Fri, Jan 10, 2014 at 05:56:25AM +0000, Mathieu Desnoyers wrote:
> Hi,
> 
> Looking at this commit:
> 
> commit f69bcbf3b4c4b333dcd7a48eaf868bf0c88edab5
> Author: Ashutosh Dixit <ashutosh.dixit@...el.com>
> Date:   Thu Sep 5 16:42:18 2013 -0700
> 
>     Intel MIC Host Driver Changes for Virtio Devices.
> 
> Especially at:
> 
> +struct mic_copy_desc {
> +#ifdef __KERNEL__
> +       struct iovec __user *iov;
> +#else
> +       struct iovec *iov;
> +#endif
> +       int iovcnt;
> +       __u8 vr_idx;
> +       __u8 update_used;
> +       __u32 out_len;
> +};
> 
> Seeing iovcnt being declared as a signed integer seems strange. The
> first question would be: why is it signed rather than unsigned ?
> 
> Then, looking further into 
> 
> drivers/misc/mic/host/mic_virtio.c:_mic_virtio_copy()
> 
> We can see that the while() loop iterates until the local variable
> iovcnt reaches the value 0 (and iovcnt is also a signed integer). If
> user-space passes e.g. INT_MIN as iovcnt field, this loop then appears
> to depend on an undefined behavior (signed underflow) to complete.
> Wouldn't it be better to use an unsigned integers both in the
> userspace API and for the local variable ?

Better yet, it should be a "__" type variable, as "int" doesn't mean
much when crossing the user/kernel boundry...

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/