Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

hidraw: fallback to HID_NAME #769

Closed
wants to merge 1 commit into from
Closed

Conversation

jo-bitsch
Copy link
Contributor

if we can't get the vendor/product string of the USB attributes, fallback to the HID_NAME string collected from the uevent.

This will be useful in linux when you provide a virtual fido2 device via /dev/uhid that emulates a USB connected HID device.

@jo-bitsch
Copy link
Contributor Author

There was a off by one in the buffer size for the string, which the pipeline runner caught. I updated the PR accordingly.

@jo-bitsch
Copy link
Contributor Author

There are 2 warning reported by the pipelines.

One is:

/home/runner/work/libfido2/libfido2/src/hid_linux.c:177:23: warning: Null pointer passed to 1st parameter expecting 'nonnull' [core.NonNullParamChecker]
  177 |                 char* first_space = strchr(hid_name, ' ');
      |                                     ^~~~~~~~~~~~~~~~~~~~~

This should be resolved by adding a hid_name != NULL to the previous condition.

The other is:

/home/runner/work/libfido2/libfido2/src/hid_linux.c:94:14: warning: Potential memory leak [unix.Malloc]
   94 |         while ((p = strsep(&cp, "\n")) != NULL && *p != '\0') {
      |                     ^~~~~~

I didn't change this line in my patch though.

This is in the context of s = cp = strdup(uevent) where later free(s); is called. I'm therefore not sure if this is a false positive. I'll think a bit more about how to best solve this.

@jo-bitsch
Copy link
Contributor Author

My bad, it was in my patch after all, just the warning was slightly misleading to me. Once I ran

scan-build-17 --use-cc=clang-17 cmake -DCMAKE_BUILD_TYPE=Debug ..

and then

 scan-build-17 --use-cc=clang-17 --status-bugs make -C build-Debug

similar to how the pipeline ran the code, the potential memory leak became apparent.

I updated the PR accordingly.

@jo-bitsch
Copy link
Contributor Author

I really appreciate the static analysis in the pipeline.

@jo-bitsch
Copy link
Contributor Author

The fuzzer discovered another issue, which I think is a false positive. Nevertheless I slightly rewrote that line in copy_info to make it clear to the compiler that di->manufacturer and di->product cannot be NULL.

Copy link
Contributor

@LDVG LDVG left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, thanks for your patch. Am I correct in understanding that you mostly want to expose a friendly name of the (virtual) HID device as created by UHID_CREATE2, e.g. when listing authenticators connected to the system?

src/hid_linux.c Outdated Show resolved Hide resolved
src/hid_linux.c Outdated Show resolved Hide resolved
@jo-bitsch
Copy link
Contributor Author

Yes, that is exactly the point: Make the listing of connected authenticators more human-friendly for virtual hid device (using /dev/uhid) or non-USB connected HID devices (if compiled with FIDO_HID_ANY)

if we can't get the vendor/product string of the USB attributes, fallback to the HID_NAME string collected from the uevent.
@jo-bitsch
Copy link
Contributor Author

I updated the PR to follow your 2 recommendations.

@LDVG
Copy link
Contributor

LDVG commented Jan 16, 2024

Paging in @martelletto: I see that something similar to this has both been added to and removed from libfido2 previously. Do you have any additional context?

@LDVG
Copy link
Contributor

LDVG commented Jan 31, 2024

We merged your changes with some additional tweaks on top. Thank you for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants