search

catalinii / minisatip

minisatip is an SATIP server for linux using local DVB-S2, DVB-C, DVB-T or ATSC cards
https://minisatip.org
328 stars 81 forks source link

Fix update_cw() validated initialization #952

Closed lars18th closed 2 years ago

lars18th commented 2 years ago

The initialization of the "validated" flag is incorrect when only one TS packet with the PUSI mark it's found. With only one positive check it's sufficient to grant that the key is correct. So, the variable requires to be iniatialized to 1 when "len > 0", and not when "len > 1". Futhermore one change in the LOG is included for improved readability.

lars18th commented 2 years ago

Hi @catalinii ,

With this patch some of my glitches with the decoding process will disappear. Please, test it. In addition, I've added a small change in the log for easier decode verification.

Regards.

lars18th commented 2 years ago

Hi @catalinii ,

The movement of the dump_cws() function it's because the call from the update_cw() when not key is found and validated. Nothing more. Detected by the Coverity checks!

lars18th commented 2 years ago

Can't really comment on the logging change, but the logic change sounds good

The logging change only does this: when a key is not found, then all keys are printed. This will help to found error/bugs/glitches with the key selection.

And regarding the logic change, I've reduced the glitches with the decoding. I'm absolutly convinced that the original author wants to check for 1 correct packet and not for more than one.

So, I hope @catalinii will merge it soon.

Jalle19 commented 2 years ago

@catalinii can we merge this?

catalinii commented 2 years ago

@lars18th I also agree that the logical change looks good.

But yes,pending is not the right word. The reason is that the key will be used regardless if it is validated or not.

Not validated may be better or just leave it as it is.

lars18th commented 2 years ago

Not validated may be better or just leave it as it is.

Done! Thank you for the comments and positive feedback. Now ready to merge.

Jalle19 commented 2 years ago

@lars18th https://github.com/Jalle19/satip-axe/issues/28 apparently this commit is causing decryption issues for many people? :thinking:

lars18th commented 2 years ago

@lars18th Jalle19/satip-axe#28 apparently this commit is causing decryption issues for many people? 🤔

Hi @Jalle19 ,

I don't think so. In which case a key can be validated 1 time and failed in with next packet? This patch has resolved a lot of troubles in my system because I use a very small buffer queue. So, only 1 PUSI packet could be checked at one shot. And then this patch provides a more stable behaviour. I'm using it for a long time!

But I can't be sure for others. All systems require to do real world checks. So, to be completly sure if this PR has some side effect, the solution is to recompile with this minimal change in the "pmt.c" file:

void update_cw(SPMT *pmt) {
    SCW *cw = NULL, *old_cw = pmt->cw;
    char buf[300];
    int64_t ctime = getTick();
    int i = 0;
    SPMT_batch start[10];
    int len = fill_packet_start_indicator(pmt->batch, pmt->blen, start, 9);
-    int validated = (len > 0);
+    int validated = (len > 1);

    // If no CW exists and we can validate the CW, try to find one again
    // Helps for CI+ case

If after this patch the behaviour not changes, then the trouble observed by others has other cause. However, if this fixes something, then I'll prepare a new PR incorporating a new option to set or modify this value. You agree with that?

Regards.

catalinii commented 2 years ago

I remember also having some instability (that's why I set to at least 2 packets with PUSI).

catalinii commented 2 years ago

Maybe we should handle separately if we have just 1 packet with PUSI, like if there is no key found fallback to non validated logic

lars18th commented 2 years ago

Hi @catalinii ,

First: Reading the post where people writes about a trouble https://github.com/Jalle19/satip-axe/issues/28 you can see this:

Are you having issues on FTA channels too or just encrypted ones?

FTA channels as well

So don't be wrong! The trouble seems to not be related with this PR.

However, regarding the idea of using at least 2 packets with PUSI for the check, let me to comment:

So if you're worried about "some instability", then perhaps it could be good to check in the function fill_packet_start_indicator() the ERROR FLAG of the header. In fact, if the source packet to check has errors, then it could generate a false positive (although with a very low probability).

What you think?

catalinii commented 2 years ago

Hi @catalinii ,

First: Reading the post where people writes about a trouble Jalle19/satip-axe#28 you can see this:

Are you having issues on FTA channels too or just encrypted ones?

FTA channels as well

So don't be wrong! The trouble seems to not be related with this PR.

However, regarding the idea of using at least 2 packets with PUSI for the check, let me to comment:

  • The basis is this: if one key can decrypt one PUSI packet, then it could decrypt any other with the same parity. It's imposible any other case (except if the packet checked has errors). Correct
  • The current logic as I can see it's to use the non-validated key, but continue asking for a new key. This causes troubles in my system if the previous returned key is correct. And for this reason I submited this patch.

If you cannot validate the key is very hard to figure out which key is correct. This could happen if the provider switches from the new to old key fore few packets.

The other issue that falls in this case is that there is one packet but that packet could have errors. Due to how dvbcsa is designed the first 8 bytes will get corrupted even if one bit is corrupted in the entire packet.

Possible solution for this case is:

lars18th commented 2 years ago

The question is not this. The question is what happens if you valite the key with 1 PUSI packet instead of 2 or more. And the answer is: nothing. It never can't happen that with two packets with the same parity one fails to decrypt and one goes right.

Well it can happen as soon as your signal is not perfect. But even decrypting one is a good indication the key is correct.

This could happen if the provider switches from the new to old key fore few packets.

And this happens. In fact I see it a lot of times. But the current code works without problems: because the old key is not expired, so when the parity change occurs, it could happen that inside a small period packets with different parity appear. So instead of 1 switch, in fact multiple happens. But as I say the code works: it changes multiple times, and all works. In fact, the problem appears when you artificially force to have 2 PUSI checks. In this case, when the flip-flop switch occurs multiple times the key search fails as only 1 packet could be check... of the old parity. And then the algorithm select the last key that perhaps is the wrong key.

Therefore, instead of thinking that the current implementation is the cause of the errors that these users comment, my feel is that the root cause is another very different. The only change that I recommend is to add the check of the packet error flag in the function fill_packet_start_indicator() because a false positive could happen. But nothing more.

Regards.

Jalle19 commented 2 years ago

A check was added in https://github.com/catalinii/minisatip/commit/c8b10b220a4650b55ad3b13ca0f64f42a3bf3096

Jalle19 commented 2 years ago

@lars18th Jalle19/satip-axe#28 apparently this commit is causing decryption issues for many people? thinking

This turned out to be a false alarm - satip-axe works correctly with v1.1.84 (which is the first release with these changes).