Skip to content

[tlul/otp_ctrl/rv_dm] improve LC gating of security critical TL-UL ports #11324

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

Merged
merged 3 commits into from
Mar 10, 2022

Conversation

msfschaffner
Copy link
Contributor

@msfschaffner msfschaffner commented Mar 9, 2022

This adds a life cycle TL-UL gating module with parameterizable number of gating muxes in each direction.
If the gate is disabled, all incoming transactions will be immediately returned with a bus error.

The gating module is then instantiated inside otp_ctrl to guard the prim port, and inside rv_dm to guard the ROM and SBA ports.

This addressed #10453 and #11296.

.lc_en_o(lc_en_buf)
);

tl_h2d_t tl_h2d_int [NumGatesPerDirection+1];
Copy link

Choose a reason for hiding this comment

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

i'm having a bit of a hard time following this.. is the idea that we need to go through multiple muxes when enabled?
So 0 takes on the input, and only 2 for example goes out?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, this implements a parameterizable number of gating muxes in each direction.

I added this due to @zi-v's request on #11296 where he mentioned that it would be good to have more than just two MUBI decoders driving muxes.

Note that multiple muxes/decoders are implemented in sequential fashion in this solution (i.e., the entire bus needs to go through the gating muxes sequentially).

It would also be possible to do a similar thing by slicing the mux into multiple shares, driven by different encoders.

Copy link

Choose a reason for hiding this comment

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

sounds good, i just wanted to make sure i understood.

///////////////////////////
// Host Side Interposing //
///////////////////////////

Copy link

Choose a reason for hiding this comment

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

is there any reason we can't use tlul_err_resp for below?
couldn't we select the error responder whenever lc is not enabled?

we would of course have to enhance it for the DataWhenError parameters.

Copy link
Contributor Author

@msfschaffner msfschaffner Mar 9, 2022

Choose a reason for hiding this comment

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

Ah! That is actually a good idea, I did not realize we had that module. Let me check how easy it would be to incorporate that.

Copy link
Contributor Author

@msfschaffner msfschaffner Mar 9, 2022

Choose a reason for hiding this comment

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

Ok I've reworked the interposer part and switched over to using tlul_err_resp.
I also made a minor update in that module so that it uses the same data for error responses as tlul_adatper_sram (based on whether this is an ifetch or not).

PTAL

end else if (!err_rsp_pending) begin
err_req_pending <= 1'b0;
end
end

assign tl_h_o_int.a_ready = ~err_rsp_pending & ~(err_req_pending & ~tl_h_i.d_ready);
assign tl_h_o_int.d_valid = err_req_pending | err_rsp_pending;
assign tl_h_o_int.d_data = '1; // Return all F
assign tl_h_o_int.d_data = (mubi4_test_true_strict(err_instr_type)) ? DataWhenInstrError :
Copy link

Choose a reason for hiding this comment

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

nice.

tl_d2h_o = '0;
tl_h2d_error = '0;

if (lc_tx_test_true_strict(lc_en_buf[2*NumGatesPerDirection]) || !tl_d2h_error.d_valid) begin
Copy link

Choose a reason for hiding this comment

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

is the second term here needed? Or is that basically defense in depth?
So if we accidentally trigger the response module for some reason, it will always have priority?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah this is only defense in depth.

Since we do not expect to switch the lc enable functionally while transactions are in flight, we technically do not need "atomic" switching where we would have to make sure that ongoing transactions are completed before switching.

Otherwise we would also have to handle the opposite case where a response to a normal non-error transaction is still pending behind the lc access gate.

Copy link
Contributor

Choose a reason for hiding this comment

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

@msfschaffner sorry I am a little bit confused here. I still see hanging cases in otp_ctrl because tl_d2h_error.d_valid is always 0 when lc_dft_en is OFF.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh shoot, I believe I got that condition wrong - it should be && instead of ||.
Let me fix that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@cindychip cindychip left a comment

Choose a reason for hiding this comment

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

lgtm

@msfschaffner msfschaffner merged commit 37f2a63 into lowRISC:master Mar 10, 2022
@msfschaffner msfschaffner deleted the lc-gate branch September 30, 2022 00:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants