[bug report] selftests/hid: add tests for bpf_hid_hw_request

[bug report] selftests/hid: add tests for bpf_hid_hw_request

[Dan Carpenter]

Hello Benjamin Tissoires,

The patch 4f7153cf461e: "selftests/hid: add tests for
bpf_hid_hw_request" from Nov 3, 2022, leads to the following Smatch
static checker warning:

	./tools/testing/selftests/hid/hid_bpf.c:214 uhid_event()
	warn: assigning (-5) to unsigned variable 'answer.u.get_report_reply.err'

./tools/testing/selftests/hid/hid_bpf.c
    203         case UHID_CLOSE:
    204                 UHID_LOG("UHID_CLOSE from uhid-dev");
    205                 break;
    206         case UHID_OUTPUT:
    207                 UHID_LOG("UHID_OUTPUT from uhid-dev");
    208                 break;
    209         case UHID_GET_REPORT:
    210                 UHID_LOG("UHID_GET_REPORT from uhid-dev");
    211 
    212                 answer.type = UHID_GET_REPORT_REPLY;
    213                 answer.u.get_report_reply.id = ev.u.get_report.id;
--> 214                 answer.u.get_report_reply.err = ev.u.get_report.rnum == 1 ? 0 : -EIO;
                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
This is a u16 so it can't hold negative error codes.

    215                 answer.u.get_report_reply.size = sizeof(feature_data);
    216                 memcpy(answer.u.get_report_reply.data, feature_data, sizeof(feature_data));
    217 
    218                 uhid_write(_metadata, fd, &answer);
    219 
    220                 break;
    221         case UHID_SET_REPORT:
    222                 UHID_LOG("UHID_SET_REPORT from uhid-dev");
    223                 break;
    224         default:

regards,
dan carpenter

[bug report] selftests/hid: add tests for bpf_hid_hw_request

[Dan Carpenter]

[ Moving these functions around made them show up as new warnings. ]

Hello Benjamin Tissoires,

Commit 4f7153cf461e ("selftests/hid: add tests for
bpf_hid_hw_request") from Nov 3, 2022 (linux-next), leads to the
following Smatch static checker warning:

	tools/testing/selftests/hid/hid_common.h:212 uhid_event()
	warn: assigning (-5) to unsigned variable 'answer.u.get_report_reply.err'

tools/testing/selftests/hid/hid_common.h
    207         case UHID_GET_REPORT:
    208                 UHID_LOG("UHID_GET_REPORT from uhid-dev");
    209 
    210                 answer.type = UHID_GET_REPORT_REPLY;
    211                 answer.u.get_report_reply.id = ev.u.get_report.id;
--> 212                 answer.u.get_report_reply.err = ev.u.get_report.rnum == 1 ? 0 : -EIO;

answer.u.get_report_reply.err is a u16.  I couldn't figure out which kind of
error codes it's supposed to store or how they're used...

    213                 answer.u.get_report_reply.size = sizeof(feature_data);
    214                 memcpy(answer.u.get_report_reply.data, feature_data, sizeof(feature_data));
    215 
    216                 uhid_write(_metadata, fd, &answer);
    217 
    218                 break;
    219         case UHID_SET_REPORT:
    220                 UHID_LOG("UHID_SET_REPORT from uhid-dev");
    221                 break;
    222         default:

regards,
dan carpenter

Re: [bug report] selftests/hid: add tests for bpf_hid_hw_request

[Benjamin Tissoires]

On Sep 05 2024, Dan Carpenter wrote:
> [ Moving these functions around made them show up as new warnings. ]

Thanks for the report Dan.

> 
> Hello Benjamin Tissoires,
> 
> Commit 4f7153cf461e ("selftests/hid: add tests for
> bpf_hid_hw_request") from Nov 3, 2022 (linux-next), leads to the
> following Smatch static checker warning:
> 
> 	tools/testing/selftests/hid/hid_common.h:212 uhid_event()
> 	warn: assigning (-5) to unsigned variable 'answer.u.get_report_reply.err'
> 
> tools/testing/selftests/hid/hid_common.h
>     207         case UHID_GET_REPORT:
>     208                 UHID_LOG("UHID_GET_REPORT from uhid-dev");
>     209 
>     210                 answer.type = UHID_GET_REPORT_REPLY;
>     211                 answer.u.get_report_reply.id = ev.u.get_report.id;
> --> 212                 answer.u.get_report_reply.err = ev.u.get_report.rnum == 1 ? 0 : -EIO;
> 
> answer.u.get_report_reply.err is a u16.  I couldn't figure out which kind of
> error codes it's supposed to store or how they're used...

It's in drivers/hid/uhid.c:262:
	req = &uhid->report_buf.u.get_report_reply;
	if (req->err) {
		ret = -EIO;
	} else {
		ret = min3(count, (size_t)req->size, (size_t)UHID_DATA_MAX);
		memcpy(buf, req->data, ret);
	}

So any non null value triggers an error, and furthermore the hardcoded
value is -EIO, which probably explains why I did not noticed it.

We should either store EIO, either a plain 1...

Cheers,
Benjamin

> 
>     213                 answer.u.get_report_reply.size = sizeof(feature_data);
>     214                 memcpy(answer.u.get_report_reply.data, feature_data, sizeof(feature_data));
>     215 
>     216                 uhid_write(_metadata, fd, &answer);
>     217 
>     218                 break;
>     219         case UHID_SET_REPORT:
>     220                 UHID_LOG("UHID_SET_REPORT from uhid-dev");
>     221                 break;
>     222         default:
> 
> regards,
> dan carpenter