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 CWHusky Support (#50) #56

Merged
merged 4 commits into from
Dec 8, 2021
Merged

Conversation

colinoflynn
Copy link
Contributor

This should add support for CW-Husky (#50). The default gain in the .yaml file should change for Husky, around 35 seemed to work for me (this could be changed in the file as well, not sure where makes sense). The CW-Lite gain is low but does work, so it's safer to keep cw-lite gain levels presumably in the code itself.

I can squash these if too many commits (especially somehow realising LFS added the full files so I had to revert them, which I'm not sure why that happened)

Copy link
Contributor

@alphan alphan left a comment

Choose a reason for hiding this comment

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

Thanks @colinoflynn, just some minor comments. It would be great if you could squash the commits.

Comment on lines +111 to +128
if not self.num_segments_min <= num_segments <= self.num_segments_max:
raise RuntimeError(
f"num_segments must be in [{self.num_segments_min}, {self.num_segments_max}]."
)
Copy link
Contributor

Choose a reason for hiding this comment

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

I haven't gotten the chance to check the husky API yet but don't we need this bounds check for husky as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll check - the husky in general does correctly support stuff (since you now specify the segment count).

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah I think it's a good idea to have the bounds check for Husky too

@@ -31,10 +31,8 @@ def create_waverunner(ot, capture_cfg):


def create_cw_lite_segmented(ot, capture_cfg):
# TODO: Remove this disconnect after removing cw-lite init from device.py.
ot.scope.dis()
return CwLiteSegmented(num_samples=capture_cfg["num_samples"],
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't this be CwSegmented(...?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Related to that, also the function name itself should maybe be changed to create_cw_segmented.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

CwSegmented

Hmm, yes thanks! Renaming files/classes was my last local step & I undid it at one point, but it left a copy of cw_lite_segmented.py still around (but with husky changes), so I didn't see this error locally. I'll double-check with a fresh checkout both are working.

Related to that, also the function name itself should maybe be changed to create_cw_segmented

Makes sense - will do that too.

Comment on lines -34 to -35
# TODO: Remove this disconnect after removing cw-lite init from device.py.
ot.scope.dis()
Copy link
Contributor

Choose a reason for hiding this comment

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

I can't remember why we had to disconnect and reconnect for cw-lite (could be because we had to re-init to be able to use segmented traces?) but it may be a good idea to handle cw-lite and husky separately here to be safe unless it's not necessary. Something like:

scope = None
if not ot.scope._is_husky:
  # CwSegmented will connect again
  ot.scope.dis()
else:
  scope = ot.scope
CwSegmented(..., scope=scope)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IIRC the issue can come up when you try talking to the CW from two different locations. The ot.scope.dis() error will hit the CW-Lite as well with new firmware however, as basically with the current USB driver the ot.scope.dis() will cause the error mentioned at newaetech/chipwhisperer#362 .

Copy link
Collaborator

@vogelpi vogelpi left a comment

Choose a reason for hiding this comment

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

Thanks for creating this PR @colinoflynn and @jpcrypt . I am excited to start working with Husky!

I have two remarks/requests:

  1. Inside ot-sca/python-requirements.txt we were pinning the ChipWhisperer version to newaetech/chipwhisperer@0998072 in order to keep segmented mode alive for CW-Lite. I would assume that for Husky we need the latest version, right? If yes, we should probably unpin the version in the requirements file.
  2. Can you please squash the commits into one?

@@ -31,10 +31,8 @@ def create_waverunner(ot, capture_cfg):


def create_cw_lite_segmented(ot, capture_cfg):
# TODO: Remove this disconnect after removing cw-lite init from device.py.
ot.scope.dis()
return CwLiteSegmented(num_samples=capture_cfg["num_samples"],
Copy link
Collaborator

Choose a reason for hiding this comment

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

Related to that, also the function name itself should maybe be changed to create_cw_segmented.

@colinoflynn
Copy link
Contributor Author

I'll leave the squashing to the last step once everything else resolved.

On python-requirements.txt I guess that raises one particularly terrible point - CW-Lite + CW-Husky require different CW versions. I'll see if our next rebuild of the CW-Lite fixes segmenting again in which case they'd both work with latest.

But in the meantime I could unpin python-requirements.txt and also add a manual version check inside of the initialisation function that warns the user if they've got an incompatible CW version for now. This would at least reduce user frustration since I think errors from wrong version are very non-obvious (especially for someone just starting).

Signed-off-by: Colin O'Flynn <coflynn@newae.com>
@alphan
Copy link
Contributor

alphan commented Oct 28, 2021

I'll leave the squashing to the last step once everything else resolved.

On python-requirements.txt I guess that raises one particularly terrible point - CW-Lite + CW-Husky require different CW versions. I'll see if our next rebuild of the CW-Lite fixes segmenting again in which case they'd both work with latest.

But in the meantime I could unpin python-requirements.txt and also add a manual version check inside of the initialisation function that warns the user if they've got an incompatible CW version for now. This would at least reduce user frustration since I think errors from wrong version are very non-obvious (especially for someone just starting).

Obviously having both working until everyone successfully transitions to CW-Husky is ideal but if this check will take considerable effort I would suggest just unpinning and printing a warning in CWSegmented::__init__(). Not sure if we need a version check since I would expect folks to update their dependencies after this PR merged or CW-Husky wouldn't work otherwise, right? We should also announce this in the SCA meeting of course.

@colinoflynn
Copy link
Contributor Author

but if this check will take considerable effort I would suggest just unpinning and printing a warning

Ah I did add that check sorry but forgot to add this back - it's not perfect as the older version doesn't support CW-Husky so it doesn't warn you about trying to use CW-Husky with the old version (but that fails anyway). I think the bigger threat is someone trying to get CW-Lite working on the "new" version, which it warns you about.

Signed-off-by: jpcrypt <jpthibault@newae.com>
@m-temp
Copy link
Collaborator

m-temp commented Nov 2, 2021

I'll leave the squashing to the last step once everything else resolved.

On python-requirements.txt I guess that raises one particularly terrible point - CW-Lite + CW-Husky require different CW versions. I'll see if our next rebuild of the CW-Lite fixes segmenting again in which case they'd both work with latest.

But in the meantime I could unpin python-requirements.txt and also add a manual version check inside of the initialisation function that warns the user if they've got an incompatible CW version for now. This would at least reduce user frustration since I think errors from wrong version are very non-obvious (especially for someone just starting).

Hi,
first of all: thanks for the work on the husky support.
Unpinning the cw version currently leads to cw 5.6.1 which requires python3.7 (at least it says so in the setup.py), which is not the default in ubuntu 18.04 LTS and thus, requires a python update. This is for sure doable, but in general ubuntu 18.04 lts + python3.6 is recommended for opentitan. Is there a version that supports the husky and python3.6? Or can the python_requires='~=3.7' be softened?

@colinoflynn
Copy link
Contributor Author

colinoflynn commented Nov 2, 2021

I checked with @alex-dewar - this is a follow-on from numpy, which started requiring 3.7 (https://numpy.org/neps/nep-0029-deprecation_policy.html). There is no critical numpy features we needed from latest, so you can just use whatever version of numpy comes with 3.6 and it should work.

Copy link
Collaborator

@m-temp m-temp left a comment

Choose a reason for hiding this comment

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

I could successfully reproduce the AES attack described at https://github.com/newaetech/ot-sca/blob/cwhusky-doc/doc/getting_started_bergen.md using the the cw310 and husky.

One minor point:

  • There is a API warning: WARNING:ChipWhisperer Other:Could not check ChipWhisperer version, error __init__() got an unexpected keyword argument 'capture_output'

Signed-off-by: jpcrypt <jpthibault@newae.com>
Signed-off-by: jpcrypt <jpthibault@newae.com>
@moidx moidx merged commit 5997c24 into lowRISC:master Dec 8, 2021
@vogelpi vogelpi mentioned this pull request Dec 24, 2021
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.

6 participants