884305 - B2G RIL: Add PIN2/PUK2 support to the emulator

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: Emulator, defect)

Description

[Gabriele Svelto [:gsvelto]]

QEMU SIM emulation currently lacks a way to check and set PIN2/PUK2 codes (also known as CHV2/UNBLOCK CHV2). Similarly setting certain properties (FDN functionality, FDN numbers, etc...) relies on CHV2 checks and should be adjusted accordingly.

Detailed information can be found in the 3GPP TS 51.011 (http://www.3gpp.org/ftp/Specs/archive/51_series/51.011/51011-500.zip) under sections 7.3, 9.2.1, 9.2.9, 9.2.10, 9.2.13, 10.5.2, 11.3.1 and 11.5.1.

Comment 1

[Gabriele Svelto [:gsvelto]]

Attachment: [WIP PATCH] Basic scaffolding for PIN2/PUK2 functionality

This adds very basic scaffolding for supporting PIN2/PUK2 functionality. I've added the required bits and pieces and I've updated the way FDN records are being written (hopefully correctly, the TSs are not exactly clear in their descriptions). What's still missing is changes to asimcard_ef_update_linear() to honor PIN2 verification if it's required.

Additionally to have complete functionality we would still need to add (in another bug?):

- CLCK command support to enable/disable FDN mode (lock/unlock in 3GPP speak)

- asimcard_ef_read_linear() and  asimcard_ef_update_linear() should prevent accessing FDN entries entirely if FDN is enabled

- changes to voice_call_event() to check if the number being dialed is in the FDN phonebook and return the appropriate failure if not (CEER should return error code 241 from what I could tell)

Comment 2

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 764767 [details] [diff] [review]
[WIP PATCH] Basic scaffolding for PIN2/PUK2 functionality

Review of attachment 764767 [details] [diff] [review]:
-----------------------------------------------------------------

Awesome job! Please also file a pull request on GitHub.

::: telephony/android_modem.c
@@ +2034,5 @@
> +        case A_SIM_STATUS_PIN2:   /* waiting for PIN2 */
> +            if ( asimcard_check_pin2( modem->sim, cmd ) )
> +                return "+CPIN: READY";
> +            else
> +                return "+CME ERROR: BAD PIN";

By now Android emulator always has +CMEE=1, which means we should always use numeric <err> in +CME response.  I know existing code does not follow this rule, but I still want to enforce numeric response in newly committed code.

You can find this in 3GPP TS 27.007 sub clause 9.2.1 "General errors".  Please use "+CME ERROR: 16" instead and have a comment right above.

@@ +2046,5 @@
> +                    return "+CPIN: READY";
> +                else
> +                    return "+CME ERROR: BAD PUK";
> +            }
> +            return "+CME ERROR: BAD PUK";

ditto

::: telephony/sim_card.c
@@ +905,4 @@
>      //   Length of BCD number/SSC contents: 7
>      //   TON and NPI: 0x81
>      // @see 3GPP TS 51.011 section 10.5.2 EFfdn
> +    ef = asimcard_ef_new_linear(0x6f3b, SIM_FILE_NEED_PIN2, 0x20);

Could you also help updating permissions of other SIM files:

  1. EFimg (4F20) => SIM_FILE_READ_ONLY | SIM_FILE_NEED_PIN
  2. EFspdi (6FCD) => SIM_FILE_READ_ONLY | SIM_FILE_NEED_PIN
  3. EFcbmid (6F48) => SIM_FILE_READ_ONLY | SIM_FILE_NEED_PIN
  4. EFcbmir (6F50) => SIM_FILE_NEED_PIN

Comment 3

[Edgar Chen [:edgar]]

Comment on attachment 764767 [details] [diff] [review]
[WIP PATCH] Basic scaffolding for PIN2/PUK2 functionality

Review of attachment 764767 [details] [diff] [review]:
-----------------------------------------------------------------

I have some comments as below. :)

::: telephony/sim_card.c
@@ +249,5 @@
> +
> +int
> +asimcard_check_pin2( ASimCard  sim, const char*  pin2 )
> +{
> +    if (sim->status != A_SIM_STATUS_PIN2)

Pin2 is only for the function that needs pin2 authentication, like update FDN. I think the status of sim and pin2 are separated. I think it should use |pin2_status| to check.

@@ +253,5 @@
> +    if (sim->status != A_SIM_STATUS_PIN2)
> +        return 0;
> +
> +    if ( !strcmp( sim->pin2, pin2 ) ) {
> +        sim->status       = A_SIM_STATUS_READY;

Pin2 should not affect the sim status.

@@ +276,5 @@
> +
> +    if ( !strcmp( sim->puk2, puk2 ) ) {
> +        strncpy( sim->puk2, puk2, A_SIM_PUK2_SIZE );
> +        strncpy( sim->pin2, pin2, A_SIM_PIN2_SIZE );
> +        sim->status       = A_SIM_STATUS_READY;

Ditto

@@ +283,5 @@
> +        return 1;
> +    }
> +
> +    if ( ++sim->puk2_retries == A_SIM_PUK2_RETRIES ) {
> +        sim->status = A_SIM_STATUS_ABSENT;

Ditto.
Even if pin2/puk2 are locked, most function still can work as normal. Pin2 only affect the function that needs pin2 authentication.

Comment 4

[Nick Thomas [:nthomas] (UTC+12)]

This bug seems to be in the wrong product/component. Perhaps somewhere in Boot2Gecko ?

Comment 5

[Gabriele Svelto [:gsvelto]]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #2)
> Awesome job! Please also file a pull request on GitHub.

Thank you, I'll make a PR as soon as I've got the finalized patch. Unless it's a problem I prefer attaching patches to the bug for reviews and follow our "traditional" flow.

> By now Android emulator always has +CMEE=1, which means we should always use
> numeric <err> in +CME response.  I know existing code does not follow this
> rule, but I still want to enforce numeric response in newly committed code.

OK, I'll leave existing code as is but I'll make sure my code always uses the codes instead. Thanks for clearing this up because I couldn't find the "string" errors in the specs and I was wondering where those were coming from.

> Could you also help updating permissions of other SIM files:
> 
>   1. EFimg (4F20) => SIM_FILE_READ_ONLY | SIM_FILE_NEED_PIN
>   2. EFspdi (6FCD) => SIM_FILE_READ_ONLY | SIM_FILE_NEED_PIN
>   3. EFcbmid (6F48) => SIM_FILE_READ_ONLY | SIM_FILE_NEED_PIN
>   4. EFcbmir (6F50) => SIM_FILE_NEED_PIN

Sure! I think I'll actually split the permissions in SIM_FILE_NEED_PIN[2]_READ and SIM_FILE_NEED_PIN[2]_UPDATE as those can be separate as per the spec. I have to double-check the encoding of the access flags though because I found the spec a bit confusing.

(In reply to Edgar Chen [:edgar][:echen] (PTO back July 2) from comment #3)
> I have some comments as below. :)

Comments are always welcome and even more so here as it's the first time I'm touching this code :)

> Pin2 is only for the function that needs pin2 authentication, like update
> FDN. I think the status of sim and pin2 are separated. I think it should use
> |pin2_status| to check.

I was wondering about that because it interacts with other functionality. The spec states that whenever a command expects a PIN (be it 1 or 2) a CPIN? command should return which one. If I split the state between PIN1 and PIN2 I could have a situation where one command need PIN1 authentication and another one PIN2 so CPIN? won't know which one was the latest. Hence I thought of encoding the querying in the current status and moving the actual session state for PIN2 (locked, unlocked, PUK2 required) in a separate field.

> Pin2 should not affect the sim status.

I imagined the following sequence:

command:  AT+CRSM=<update an FDN field>
response: +CME ERROR: 17 (PIN2 required)

-> at this stage the SIM is waiting for the PIN2 to be entered hence we need to record this state

command:  AT+CPIN?
response: +CPIN: SIM PIN2
command:  AT+CPIN=<PIN2>
response; +CPIN: READY

-> at this stage a new AT+CPIN? command should return again +CPIN: READY as we're not waiting for a PIN anymore

Does this make sense to you?

> > +    if ( ++sim->puk2_retries == A_SIM_PUK2_RETRIES ) {
> > +        sim->status = A_SIM_STATUS_ABSENT;
> 
> Ditto.
> Even if pin2/puk2 are locked, most function still can work as normal. Pin2
> only affect the function that needs pin2 authentication.

This is something that wasn't clear to me. So if we run out of PIN2/PUK2 retries what happens is that only PIN2 functionality is permanently disabled, correct? I'll add a state for that and adjust the existing code.

Comment 6

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH] Add support for PIN2/PUK2 functionality

I finally had some time to come back here and to finalize the functionality. I tried to address the issues that Edgar pointed out in comment 3 by decoupling the PIN2 functionality from the SIM state except for the PIN2-required / PUK2-required states that are needed. Those states however always get you back to a SIM-ready state and this is actually documented behavior in TS 27.007 section 8.3:

«
SIM PIN2

MT is waiting SIM PIN2 to be given (this <code> is recommended to be returned only when the last executed command resulted in PIN2 authentication failure (i.e. +CME ERROR: 17); if PIN2 is not entered right after the failure, it is recommended that MT does not block its operation)

SIM PUK2

MT is waiting SIM PUK2 to be given (this <code> is recommended to be returned only when the last executed command resulted in PUK2 authentication failure (i.e. +CME ERROR: 18); if PUK2 and new PIN2 are not entered right after the failure, it is recommended that MT does not block its operation)
»

I've also fixed the access permissions as per comment 2 and modified the SIM I/O functions to honor them (previously the file access permissions were ignored save for the read-only flag).

I'm not asking for review just yet because I haven't had the time to thoroughly test the new functionality. Additionally this is still not enough to fully test FDN functionality in the emulator. For that we'll need the AT+CLCK command and modifications to voice_call_event(); I'll do this stuff in a follow-up however.

Comment 7

[Gabriele Svelto [:gsvelto]]

Attachment: Pointer to the GitHub pull-request

Comment 8

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 779849 [details] [diff] [review]
[PATCH] Add support for PIN2/PUK2 functionality

Review of attachment 779849 [details] [diff] [review]:
-----------------------------------------------------------------

Hi Gabriele,

The root problem in supporting PIN2/PUK2 is AT command CPIN? returns "SIM PIN2" or "SIM PUK2" if the last SIM PIN2/PUK2 related operation was rejected, or returns "SIM PIN/PUK" if it's waiting for SIM PIN/PUK.  For example,

  command:  AT+CRSM=<update an FDN field>
  response: +CME ERROR: 17 (PIN2 required)
  command:  AT+CPIN?
  response: +CPIN: SIM PIN2

  command:  AT+CRSM=<read iccid>
  response: +CRSM: ....
  command:  AT+CPIN?
  response: +CPIN: READY

  command:  AT+CRSM=<read cbmi>
  response: +CME ERROR: 11 (PIN required)
  command:  AT+CPIN?
  response: +CPIN: SIM PIN

  command:  AT+CRSM=<update an FDN field>
  response: +CME ERROR: 17 (PIN2 required)
  command:  AT+CPIN?
  response: +CPIN: SIM PIN2

This brings great problem in getSIMStatus() in hardware/ril because it assumes we could only have "SIM PIN/PUK" returned.  So if the pull request you have here doesn't include a fix to hardware/ril, that's probably insufficient.

::: telephony/android_modem.c
@@ +1539,5 @@
>          case A_SIM_STATUS_NOT_READY: answer = "+CMERROR: NOT READY"; break;
>          case A_SIM_STATUS_PIN:       answer = "+CPIN: SIM PIN"; break;
>          case A_SIM_STATUS_PUK:       answer = "+CPIN: SIM PUK"; break;
> +        case A_SIM_STATUS_PIN2:      answer = "+CPIN: SIM PIN2"; break;
> +        case A_SIM_STATUS_PUK2:      answer = "+CPIN: SIM PUK2"; break;

|A_SIM_STATUS_PIN2| and |A_SIM_STATUS_PUK2| is never assigned.  Besides, if we return "SIM PIN2" or "SIM PUK2" here, we'll find SIM status becomes SIM_ABSENT in |getSIMStatus()| in hardware/ril[1].

[1]: https://github.com/mozilla-b2g/platform_hardware_ril/blob/master/reference-ril/reference-ril.c#L2862

@@ +2031,4 @@
>              }
>              return "+CME ERROR: BAD PUK";
>  
> +        case A_SIM_STATUS_PIN2:   /* waiting for PIN2 */

|A_SIM_STATUS_PIN2| is never assigned to |sim->status|, so |asimcard_get_status()| will never return |A_SIM_STATUS_PIN2|.  The lines here are basically dead.

@@ +2036,5 @@
> +                return "+CPIN: READY";
> +            else
> +                return "+CME ERROR: 16";
> +
> +        case A_SIM_STATUS_PUK2:   /* waiting for PUK2 */

ditto.

::: telephony/sim_card.c
@@ +261,5 @@
> +     * retries */
> +    if (sim->pin2_status == A_SIM_PIN2_DISABLED ||
> +        sim->pin2_status == A_SIM_PUK2_LOCKED   ||
> +        sim->status != A_SIM_STATUS_PIN2        )
> +        return 0;

So this is always true, always returns 0.

@@ +625,5 @@
>      }
>  
> +    if (ef->any.flags & SIM_FILE_NEED_PIN &&
> +        sim->status != A_SIM_STATUS_READY )
> +    {

opening brace in the same line with |if|.

Comment 9

[Gabriele Svelto [:gsvelto]]

Thank you very much for your feedback!

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #8)
> The root problem in supporting PIN2/PUK2 is AT command CPIN? returns "SIM
> PIN2" or "SIM PUK2" if the last SIM PIN2/PUK2 related operation was
> rejected, or returns "SIM PIN/PUK" if it's waiting for SIM PIN/PUK.  For
> example,
> 
>   command:  AT+CRSM=<update an FDN field>
>   response: +CME ERROR: 17 (PIN2 required)
>   command:  AT+CPIN?
>   response: +CPIN: SIM PIN2
> 
>   command:  AT+CRSM=<read iccid>
>   response: +CRSM: ....
>   command:  AT+CPIN?
>   response: +CPIN: READY
> 
>   command:  AT+CRSM=<read cbmi>
>   response: +CME ERROR: 11 (PIN required)
>   command:  AT+CPIN?
>   response: +CPIN: SIM PIN
> 
>   command:  AT+CRSM=<update an FDN field>
>   response: +CME ERROR: 17 (PIN2 required)
>   command:  AT+CPIN?
>   response: +CPIN: SIM PIN2

Yes, one of the aspects that are not clear to me is what "clears" this status. So basically my question is: is the SIM PIN2/PUK2 status sticky or not? For example if I do the following sequence:

  command:  AT+CRSM=<update an FDN field>
  response: +CME ERROR: 17 (PIN2 required)
  command:  AT+CPIN?
  response: +CPIN: SIM PIN2
  command:  AT+CPIN?

Is the second AT+CPIN? command going to clear the PIN2 status and return +CPIN: READY or is it still going to return +CPIN: SIM PIN2? From my perspective that dictates how I should implement tracking the PIN2 status. If that status is not sticky then it's just a matter of having a flag that tells me if the previous operation required PIN2 authentication or not and clear it upon receiving any new operation. If it is sticky on the other hand (i.e. in the example above the second AT+CPIN? also returns +CPIN: SIM PIN2?) then it's more complicated.

> This brings great problem in getSIMStatus() in hardware/ril because it
> assumes we could only have "SIM PIN/PUK" returned.  So if the pull request
> you have here doesn't include a fix to hardware/ril, that's probably
> insufficient.

I wasn't aware of that code yet, thanks for pointing it out. I will definitely prepare a second PR to add support to the reference RIL implementation too.

> |A_SIM_STATUS_PIN2| and |A_SIM_STATUS_PUK2| is never assigned.  Besides, if
> we return "SIM PIN2" or "SIM PUK2" here, we'll find SIM status becomes
> SIM_ABSENT in |getSIMStatus()| in hardware/ril[1].
>
> [snip]
>
> So this is always true, always returns 0.

Right, the only code that would trigger that condition now is in asimcard_io_update_record() when trying to update a record that requires PIN2 authentication. The reason I didn't implement it is that I was unsure of how to track that status as I've explained above.

> opening brace in the same line with |if|.

I'll modify the patch to better fit the rest of the file. BTW I had a question on this topic: the upstream coding conventions of QEMU seem to differ quite a bit from what we're doing in these files. Is this OK? If we'd ever want to send some of these changes upstream I think this might be a problem but then again maybe we don't want to send any of this upstream; I just don't know if that's the case.

Comment 10

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Gabriele Svelto [:gsvelto] from comment #9)
> > opening brace in the same line with |if|.
> 
> I'll modify the patch to better fit the rest of the file. BTW I had a
> question on this topic: the upstream coding conventions of QEMU seem to
> differ quite a bit from what we're doing in these files. Is this OK? If we'd
> ever want to send some of these changes upstream I think this might be a
> problem but then again maybe we don't want to send any of this upstream; I
> just don't know if that's the case.

If I got some time, which every developer says every two or three days during his life time, I would really want to rebase our changes to official qemu instead of Android's.  And I believe Android developers are thinking about the same thing :)

The coding style differences you see here, are probably from Android developers.  We just keep the same style of the editing file, not all the files.  And since we mostly changes telephony/*, we follow Android's style most of the time.

Comment 11

[Gabriele Svelto [:gsvelto]]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #10)
> If I got some time, which every developer says every two or three days
> during his life time, I would really want to rebase our changes to official
> qemu instead of Android's.  And I believe Android developers are thinking
> about the same thing :)

Sounds good, I can help you out on this endeavor if you feel like you could use a hand.

> The coding style differences you see here, are probably from Android
> developers.  We just keep the same style of the editing file, not all the
> files.  And since we mostly changes telephony/*, we follow Android's style
> most of the time.

Got it. I'm prepping a new version which uses the 'request-PIN2-only-after-an-operation-failed-and-then-clear-it' approach which also fixes the issue of never setting in when responding to AT+CRSM commands touching FDN entries.