In function size_from_channelarray(), the return value 'bytes' is defined
as int type. However, the calcution of 'bytes' in this function is designed
to use the unsigned int type. So it is necessary to change 'bytes' type to
unsigned int to avoid integer overflow.
The size_from_channelarray() is called in main() function, its return value
is directly multipled by 'buf_len' and then used as the malloc() parameter.
The 'buf_len' is completely controllable by user, thus a multiplication
overflow may occur here. This could allocate an unexpected small area.
Signed-off-by: Chenyuan Mi <[email protected]>
---
tools/iio/iio_generic_buffer.c | 15 +++++++++++----
1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/tools/iio/iio_generic_buffer.c b/tools/iio/iio_generic_buffer.c
index f8deae4e26a1..dc7d19c179ca 100644
--- a/tools/iio/iio_generic_buffer.c
+++ b/tools/iio/iio_generic_buffer.c
@@ -51,9 +51,9 @@ enum autochan {
* Has the side effect of filling the channels[i].location values used
* in processing the buffer output.
**/
-static int size_from_channelarray(struct iio_channel_info *channels, int num_channels)
+static unsigned int size_from_channelarray(struct iio_channel_info *channels, int num_channels)
{
- int bytes = 0;
+ unsigned int bytes = 0;
int i = 0;
while (i < num_channels) {
@@ -348,7 +348,7 @@ int main(int argc, char **argv)
ssize_t read_size;
int dev_num = -1, trig_num = -1;
char *buffer_access = NULL;
- int scan_size;
+ unsigned int scan_size;
int noevents = 0;
int notrigger = 0;
char *dummy;
@@ -674,7 +674,14 @@ int main(int argc, char **argv)
}
scan_size = size_from_channelarray(channels, num_channels);
- data = malloc(scan_size * buf_len);
+
+ size_t total_buf_len = scan_size * buf_len;
+
+ if (scan_size > 0 && total_buf_len / scan_size != buf_len) {
+ ret = -EFAULT;
+ perror("Integer overflow happened when calculate scan_size * buf_len");
+ goto error;
+ }
+
+ data = malloc(total_buf_len);
if (!data) {
ret = -ENOMEM;
goto error;
--
2.25.1
On Tue, 18 Jul 2023 08:15:42 +0000
Chenyuan Mi <[email protected]> wrote:
Hi,
In principle I support hardening code, though in this case we are talking
about example code only. We have libiio and similar for anyone who wants
to do more than basic tests.
> In function size_from_channelarray(), the return value 'bytes' is defined
> as int type. However, the calcution of 'bytes' in this function is designed
> to use the unsigned int type. So it is necessary to change 'bytes' type to
> unsigned int to avoid integer overflow.
For this one, in practice it's controlled entirely by the kernel drivers
and they won't get anywhere near integer overflow. The change is
small however and doesn't hurt readability so I guess no harm applying it.
>
> The size_from_channelarray() is called in main() function, its return value
> is directly multipled by 'buf_len' and then used as the malloc() parameter.
> The 'buf_len' is completely controllable by user, thus a multiplication
> overflow may occur here. This could allocate an unexpected small area.
That would have to be a very large allocation... I suppose it is possible
someone might try it...
>
> Signed-off-by: Chenyuan Mi <[email protected]>
My first inclination is not to apply this on basis that it adds slight complexity
to example code (the aim of which is too illustrate the interface), however on the other
side of things the checks don't add significant complexity...
So I tried to apply it, but it doesn't go on cleanly and patch is telling
me it's malformed. I'm not quite sure why.
patching file tools/iio/iio_generic_buffer.c
patch: **** malformed patch at line 68: ret = -ENOMEM;
Jonathan
> ---
> tools/iio/iio_generic_buffer.c | 15 +++++++++++----
> 1 file changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/tools/iio/iio_generic_buffer.c b/tools/iio/iio_generic_buffer.c
> index f8deae4e26a1..dc7d19c179ca 100644
> --- a/tools/iio/iio_generic_buffer.c
> +++ b/tools/iio/iio_generic_buffer.c
> @@ -51,9 +51,9 @@ enum autochan {
> * Has the side effect of filling the channels[i].location values used
> * in processing the buffer output.
> **/
> -static int size_from_channelarray(struct iio_channel_info *channels, int num_channels)
> +static unsigned int size_from_channelarray(struct iio_channel_info *channels, int num_channels)
> {
> - int bytes = 0;
> + unsigned int bytes = 0;
> int i = 0;
>
> while (i < num_channels) {
> @@ -348,7 +348,7 @@ int main(int argc, char **argv)
> ssize_t read_size;
> int dev_num = -1, trig_num = -1;
> char *buffer_access = NULL;
> - int scan_size;
> + unsigned int scan_size;
> int noevents = 0;
> int notrigger = 0;
> char *dummy;
> @@ -674,7 +674,14 @@ int main(int argc, char **argv)
> }
>
> scan_size = size_from_channelarray(channels, num_channels);
> - data = malloc(scan_size * buf_len);
> +
> + size_t total_buf_len = scan_size * buf_len;
> +
> + if (scan_size > 0 && total_buf_len / scan_size != buf_len) {
> + ret = -EFAULT;
> + perror("Integer overflow happened when calculate scan_size * buf_len");
> + goto error;
> + }
> +
> + data = malloc(total_buf_len);
> if (!data) {
> ret = -ENOMEM;
> goto error;
Chenyuan Mi wrote:
> Hi,
>
> In principle I support hardening code, though in this case we are talking about example code only. We have libiio and similar for anyone who wants to do more than basic tests.
>
> > In function size_from_channelarray(), the return value 'bytes' is
> > defined as int type. However, the calcution of 'bytes' in this
> > function is designed to use the unsigned int type. So it is necessary
> > to change 'bytes' type to unsigned int to avoid integer overflow.
>
> For this one, in practice it's controlled entirely by the kernel drivers and they won't get anywhere near integer overflow. The change is small however and doesn't hurt readability so I guess no harm applying it.
>
> >
> > The size_from_channelarray() is called in main() function, its return
> > value is directly multipled by 'buf_len' and then used as the malloc() parameter.
> > The 'buf_len' is completely controllable by user, thus a
> > multiplication overflow may occur here. This could allocate an unexpected small area.
>
> That would have to be a very large allocation... I suppose it is possible someone might try it...
>
> >
> > Signed-off-by: Chenyuan Mi <[email protected]>
>
> My first inclination is not to apply this on basis that it adds slight complexity to example code (the aim of which is too illustrate the interface), however on the other side of things the checks don't add significant complexity...
>
> So I tried to apply it, but it doesn't go on cleanly and patch is telling me it's malformed. I'm not quite sure why.
>
> patching file tools/iio/iio_generic_buffer.c
> patch: **** malformed patch at line 68: ret = -ENOMEM;
>
> Jonathan
I re-send a patch v2, and test patch v2 to make sure it can be applied. Thank you!