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

Add ccid apps to the usbip runner #149

Merged
merged 16 commits into from
Jan 25, 2023
Merged

Add ccid apps to the usbip runner #149

merged 16 commits into from
Jan 25, 2023

Conversation

sosthene-nitrokey
Copy link
Collaborator

@sosthene-nitrokey sosthene-nitrokey commented Jan 11, 2023

This PR cleans up usbd-ccid a bit and adds the CCID apps (opcard) to the usbip runner.

Closes #140, #150 and #151

On my machine I enconter 2 bugs:

  • pcscd and this somehow manage to trigger a kernel bug:

    1. `cargo run --release --features alpha
    2. usbip attach -r localhost -b 1-1 (enough times for it to not return immediately)
    3. opgpcard status
    4. now lsusb hangs and dmesg shows an error: kernel BUG at mm/usercopy.c:101!
  • gpg still fails to detect this as an openpgp smartcard for some reason

@robin-nitrokey
Copy link
Member

Regarding the first problem: I think I can reproduce this. opgpcard hangs, and subsequent usbip calls don’t seem to work at all.

@sosthene-nitrokey
Copy link
Collaborator Author

Does lsusb work after that?

@robin-nitrokey
Copy link
Member

No.

@sosthene-nitrokey
Copy link
Collaborator Author

Anything suspicious in dmesg?

@robin-nitrokey
Copy link
Member

Yes, basically the same one you had:

usercopy: Kernel memory exposure attempt detected from SLUB object 'kmalloc-16' (offset 0, size 64)!
kernel BUG at mm/usercopy.c:99

@sosthene-nitrokey
Copy link
Collaborator Author

sosthene-nitrokey commented Jan 12, 2023

There appear to be a similar bug already reported: https://bugzilla.kernel.org/show_bug.cgi?id=215487

The symptoms are the same, but the dmesg error is not.

@sosthene-nitrokey
Copy link
Collaborator Author

I included a fix for #151

@sosthene-nitrokey
Copy link
Collaborator Author

Includes a fix for #150

Copy link
Member

@robin-nitrokey robin-nitrokey left a comment

Choose a reason for hiding this comment

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

LGTM!

components/usbd-ccid/src/pipe.rs Outdated Show resolved Hide resolved
components/usbd-ccid/src/pipe.rs Outdated Show resolved Hide resolved
@sosthene-nitrokey
Copy link
Collaborator Author

It looks like ddce9f8 fixing #151 actually breaks the firmware.

When using highspeed usb, some calculations worked with usb 1 constants.
This caused packets to be handled improperly.
@sosthene-nitrokey
Copy link
Collaborator Author

sosthene-nitrokey commented Jan 23, 2023

Ok, so the the way it was done before the fix for #151 is correct, because the response blocks can be sent in multiple parts and it's just that the gnuk test suite doesn't support this.

I still don't understand exactly how my fix made thing worse for the firmware itself, but for now I'll revert it. We should consider fixing the gnuk test suite to use it.

Edit: see #151 (comment)

Copy link
Member

@robin-nitrokey robin-nitrokey left a comment

Choose a reason for hiding this comment

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

Not tested yet, but the code changes look good to me.

@sosthene-nitrokey
Copy link
Collaborator Author

I think we should merge this now. It works well on the firmware itself. I'll then upstream the usbd-ccid changes into https://github.com/trussed-dev/usbd-ccid

@sosthene-nitrokey sosthene-nitrokey merged commit c494cc4 into main Jan 25, 2023
@daringer daringer deleted the usbip-ccid branch February 6, 2023 21:33
@ZenithalHourlyRate
Copy link

Regarding usbip, I think I have found a bug in your usbip server, see https://lore.kernel.org/lkml/ZBHxfUX60EyCMw5l@Sun/

@robin-nitrokey
Copy link
Member

@ZenithalHourlyRate Thank you for looking into the bug report! I will try to fix the issue you pointed out.

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

Successfully merging this pull request may close these issues.

usbip-runner: Add CCID support
3 participants