num_req_outstanding might overflow in tlul_socket_1n

[nodix95]

Suppose a host has one outstanding request to some peripheral. In that case, the host's 1:N socket will have its num_req_outstanding set to "1" and dev_select_outstanding pointing to the appropriate device. The device responds by raising the d_valid signal, which is considered accepted if the host's d_ready signal is also HIGH. If the response is accepted, num_req_outstanding decrements to "0", and assuming the host is not making any new requests, dev_select_outstanding retains its previous value.

If the peripheral continues holding the d_valid signal HIGH, a new "response" might be registered by the socket if the d_ready signal is still HIGH (as is always the case with e.g. the main core). This is because the response routing logic only takes the dev_select_outstanding signal into account:

  always_comb begin
    tl_t_p = tl_u_i[N];
    for (int idx = 0 ; idx < N ; idx++) begin
      if (dev_select_outstanding == NWD'(idx)) tl_t_p = tl_u_i[idx];
    end
  end

This extra response will then get registered as accepted and gets forwarded to the host. More importantly, the num_req_outstanding gets decremented again resulting in an overflow, which then raises the hold_all_requests signal, essentially blocking the host from making any further requests to the crossbar.

I have managed to create this scenario by adding a corrupted peripheral to the system for the purposes of my research:

Even though this behavior is not anticipated from any of the peripheral devices during normal operation, I think a small change of the response routing logic code would add to the security of the system, like for example:

  always_comb begin
    tl_t_p = tl_u_i[N];
    for (int idx = 0 ; idx < N ; idx++) begin
      if (dev_select_outstanding == NWD'(idx) && num_req_outstanding != '0) tl_t_p = tl_u_i[idx];
    end
  end

[tjaychen]

that's a reasonable change although I'm not quite sure what security this would add to the system.
Are you worried about a denial of service..?

[nodix95]

Yes, exactly. Denial of service of a host in the system.

[eunchan]

The attack scenario, even with the changes, cannot be prevented in my opinion. As d_valid somehow was corrupted already, the transactions are not paired anymore.

The list goes on and on to completely block this attack. I think it would be simpler to rely on Watchdog timer in this case as the crossbar hangs.

[tjaychen]

yeah i tend to agree with @eunchan. The denial of service of type attacks are not something we really cover.
Because someone could attack the clock, the processor, the memory etc and the results would all be the same.

Still I think this particular enhancement is not cumbersome, i could just absorb it.

[msfschaffner]

@msfschaffner

[tjaychen]

This is not something we are going to address for M3 since it is a guard against something that is not really in scope.
The thread is acknowledged, but we will probably want to find a more holistic ways to tackle issues like these.

[andreaskurth]

Triaged for tlul. Keeping Type:FutureRelease Not relevant to currently planned releases/milestones as per the last comment.

[vogelpi]

I am closing this issue as not planned as it adds marginal defense against something that is not in scope anyway (denial of service on bus).