linux-kernel - Re: [PATCH 07/17] usb: host: ehci-dbg: fix unsigned comparison

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:	Mon, 4 Jan 2016 15:50:45 -0500 (EST)
From:	Alan Stern <stern@...land.harvard.edu>
To:	"Geyslan G. Bem" <geyslan@...il.com>
cc:	linux-kernel@...r.kernel.org,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	<linux-usb@...r.kernel.org>
Subject: Re: [PATCH 07/17] usb: host: ehci-dbg: fix unsigned comparison

On Mon, 4 Jan 2016, Geyslan G. Bem wrote:

> This patch fixes an unsigned comparison to less than 0.

No, it doesn't.  It changes an unsigned comparison for less than or 
equal to 0, which is very different from less than 0.

> Signed-off-by: Geyslan G. Bem <geyslan@...il.com>
> ---
> 
> Notes:
>     I'm not sure about that comparison because in qh_lines() temp receives
>     the snprintf() return and thereafter occurs this comparison:
>     
>     if (size < temp)
>        	   temp = size;
>     
>     Is it a test of string truncation right? That possibility is being
>     treated. But if after some snprintf returns the temp is exactly size
>     minus 1 (trailing null)? Could this scenario happen? If yes, I think
>     size could be not counting correctly. Let me know more about it.

I think the two weird code sequences in qh_lines() were written before 
scnprintf existed.  They should be changed to use scnprintf instead of 
snprintf; then the "if (size < temp) temp = size;" things can be 
removed.

>  drivers/usb/host/ehci-dbg.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/host/ehci-dbg.c b/drivers/usb/host/ehci-dbg.c
> index 980ca55..1645120 100644
> --- a/drivers/usb/host/ehci-dbg.c
> +++ b/drivers/usb/host/ehci-dbg.c
> @@ -542,7 +542,7 @@ static ssize_t fill_async_buffer(struct debug_buffer *buf)
>  		next += temp;
>  
>  		list_for_each_entry(qh, &ehci->async_unlink, unlink_node) {
> -			if (size <= 0)
> +			if (size == 0)
>  				break;
>  			qh_lines(ehci, qh, &next, &size);
>  		}

The new line does exactly the same thing as the old line.  There's no 
reason to make this change.

Alan Stern

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