linux-kernel - Re: [PATCH for-5.19/uclogic] HID: uclogic: Remove useless loop

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] [day] [month] [year] [list]

Date:   Mon, 30 May 2022 19:33:06 +0200
From:   José Expósito <jose.exposito89@...il.com>
To:     Stefan Berzl <stefanberzl@...il.com>
Cc:     jikos@...nel.org, benjamin.tissoires@...hat.com, spbnick@...il.com,
        linux-input@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH for-5.19/uclogic] HID: uclogic: Remove useless loop

On Mon, May 30, 2022 at 06:46:38PM +0200, Stefan Berzl wrote:
> Think about what this behavior really achieves. In the first iteration,
> we check if params->pen.id equals the report_id, which is the actual
> report id from the usb message. If that is the case, we check if the
> second byte of the message is such that we need an updated "subreport"
> for this particular message. Therefore, the report_id is set to the
> subreport->id. This subreport->id is by design supposed to be different
> from the original params->pen.id, because otherwise, why would we need  
> this update? If we then "continue" with this useless loop, either one of
> two cases can happen:
> 
> The best case is that the (report_id = subreport->id) != params->pen.id 
> in which case the if-block won't be executed and we only wasted time.
> 
> If the (report_id = subreport->id) == params->pen.id however, things get
> interesting. The "subreport_list_end" and "subreport" variables will
> again be set to entries based on "params->pen.subreport_list", which is
> totally unchanged from the last iteration. We will iterate the same
> subreports, find the same result, set report_id to the same
> subreport->id and, that's the beauty of it, "continue" this ingenious
> loop, creating an infinite loop.

True, data[1] doesn't change, so an extra if is executed for no reason.
I mean, it is not dramatic, but I guess the while loop could be removed
for clarity.

I wonder why it was implemented in a loop though, check commit
8b013098be ("HID: uclogic: Replace pen_frame_flag with subreport_list").

The while loop is intrudeced there and I can imagine that for a good
reason... However, I can not think in a case where removing the loop
could cause issues.

> This contraption is in the best case only wasteful, yet it has been
> accepted all willy-nilly like. Really gets the noggin joggin.
> 
> > 
> >> -			} else {
> >> -				return uclogic_raw_event_pen(drvdata, data, size);
> >> +	/* Tweak pen reports, if necessary */
> >> +	if ((report_id == params->pen.id) && (size >= 2)) {
> >> +		subreport_list_end =
> >> +			params->pen.subreport_list +
> >> +			ARRAY_SIZE(params->pen.subreport_list);
> >> +		/* Try to match a subreport */
> >> +		for (subreport = params->pen.subreport_list;
> >> +		     subreport < subreport_list_end; subreport++) {
> >> +			if (subreport->value != 0 &&
> >> +			    subreport->value == data[1]) {
> >> +				break;
> >>  			}
> >>  		}
> >> -
> >> -		/* Tweak frame control reports, if necessary */
> >> -		for (i = 0; i < ARRAY_SIZE(params->frame_list); i++) {
> >> -			if (report_id == params->frame_list[i].id) {
> >> -				return uclogic_raw_event_frame(
> >> -					drvdata, &params->frame_list[i],
> >> -					data, size);
> >> -			}
> >> +		/* If a subreport matched */
> >> +		if (subreport < subreport_list_end) {
> >> +			/* Change to subreport ID, and restart */
> >> +			report_id = data[0] = subreport->id;
> >> +		} else {
> >> +			return uclogic_raw_event_pen(drvdata, data, size);
> >>  		}
> >> +	}
> >>  
> >> -		break;
> >> +	/* Tweak frame control reports, if necessary */
> >> +	for (i = 0; i < ARRAY_SIZE(params->frame_list); i++) {
> >> +		if (report_id == params->frame_list[i].id) {
> >> +			return uclogic_raw_event_frame(
> >> +				drvdata, &params->frame_list[i],
> >> +				data, size);
> >> +		}
> >>  	}
> >>  
> >>  	return 0;
> >> -- 
> >> 2.36.1
> >>
> >>
> 
> Bye bye
> 
> Stefan Berzl