diff mbox series

qmi: sim: implement lock(LockPin method)

Message ID 1731845036-11874-1-git-send-email-ivo.g.dimitrov.75@gmail.com (mailing list archive)
State Changes Requested, archived
Headers show
Series qmi: sim: implement lock(LockPin method) | expand

Commit Message

Ivaylo Dimitrov Nov. 17, 2024, 12:03 p.m. UTC 
---
 drivers/qmimodem/sim.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/qmimodem/uim.h |  1 +
 2 files changed, 71 insertions(+)

Comments

Marcel Holtmann Nov. 18, 2024, 10:13 a.m. UTC | #1 
Hi Ivaylo,

<please insert commit description here>


> On Nov 17, 2024, at 13:03, Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com> wrote:
> 
> ---
> drivers/qmimodem/sim.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++++++
> drivers/qmimodem/uim.h |  1 +
> 2 files changed, 71 insertions(+)
> 
> diff --git a/drivers/qmimodem/sim.c b/drivers/qmimodem/sim.c
> index b1d8f22..65c0fa6 100644
> --- a/drivers/qmimodem/sim.c
> +++ b/drivers/qmimodem/sim.c
> @@ -844,6 +844,75 @@ static void qmi_query_locked(struct ofono_sim *sim,
> l_free(cbd);
> }
> 
> +static void qmi_lock(struct ofono_sim *sim,
> + enum ofono_sim_password_type passwd_type,
> + int enable, const char *passwd,
> + ofono_sim_lock_unlock_cb_t cb, void *user_data)
> +{
> + struct sim_data *data = ofono_sim_get_data(sim);
> + struct cb_data *cbd = cb_data_new(cb, user_data);
> + int passwd_len;
> + uint16_t info_len;
> + uint8_t pin_id;
> + struct qmi_param *param;
> + uint8_t session[2];
> + struct {
> + uint8_t id;
> + uint8_t enabled;
> + uint8_t length;
> + uint8_t pin[0];
> + } __attribute__((__packed__)) *info;

Is this a good idea to define this inline with the function and not better as some QMI generic define?

Regards

Marcel
Denis Kenzior Nov. 18, 2024, 4:54 p.m. UTC | #2 
Hi Ivo,

On 11/17/24 6:03 AM, Ivaylo Dimitrov wrote:
> ---
>   drivers/qmimodem/sim.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++++++
>   drivers/qmimodem/uim.h |  1 +
>   2 files changed, 71 insertions(+)

Looks like this patch is on top of the query_facility_lock.  Please send these 
as a series in the future, otherwise the CI gets confused.

> 
> diff --git a/drivers/qmimodem/sim.c b/drivers/qmimodem/sim.c
> index b1d8f22..65c0fa6 100644
> --- a/drivers/qmimodem/sim.c
> +++ b/drivers/qmimodem/sim.c
> @@ -844,6 +844,75 @@ static void qmi_query_locked(struct ofono_sim *sim,
>   	l_free(cbd);
>   }
>   
> +static void qmi_lock(struct ofono_sim *sim,
> +		enum ofono_sim_password_type passwd_type,
> +		int enable, const char *passwd,
> +		ofono_sim_lock_unlock_cb_t cb, void *user_data)
> +{
> +	struct sim_data *data = ofono_sim_get_data(sim);
> +	struct cb_data *cbd = cb_data_new(cb, user_data);
> +	int passwd_len;
> +	uint16_t info_len;
> +	uint8_t pin_id;
> +	struct qmi_param *param;
> +	uint8_t session[2];
> +	struct {
> +		uint8_t id;
> +		uint8_t enabled;
> +		uint8_t length;
> +		uint8_t pin[0];
> +	} __attribute__((__packed__)) *info;
> +
> +	DBG("");
> +
> +	switch (passwd_type) {
> +	case OFONO_SIM_PASSWORD_SIM_PIN:
> +		pin_id = 0x01;
> +		break;
> +	case OFONO_SIM_PASSWORD_SIM_PIN2:
> +		pin_id = 0x02;
> +		break;
> +	default:
> +		goto error;
> +	}
> +
> +	if (!passwd)
> +		goto error;
> +
> +	passwd_len = strlen(passwd);
> +
> +	if (passwd_len <= 0 || passwd_len > 0xFF)

strlen returns a size_t, so checking < 0 doesn't really make sense.  Also, the 
core already performs validation of the pin using 'is_valid_pin'.  So I'm not 
really sure any of this validation is needed?

> +		goto error;
> +
> +	param = qmi_param_new();
> +
> +	/* info */
> +	info_len = sizeof(*info) + passwd_len;
> +	info = alloca(info_len);
> +	info->id = pin_id;
> +	info->enabled = enable ? 0x01 : 0x00;
> +	info->length = (uint8_t) passwd_len;
> +	memcpy(info->pin, passwd, passwd_len);
> +
> +	qmi_param_append(param, QMI_UIM_PARAM_MESSAGE_INFO, info_len, info);
> +
> +	/* session */
> +	session[0] = 0x6; /* card on slot 1 */
> +	session[1] = 0x0;

Hmm, can we get some #defines for these magic numbers? Or static const variable 
declaration?

> +	qmi_param_append(param, QMI_UIM_PARAM_MESSAGE_SESSION_INFO,
> +					sizeof(session), session);
> +
> +	if (qmi_service_send(data->uim, QMI_UIM_ENABLE_PIN, param,
> +					pin_send_cb, cbd, cb_data_unref) > 0)
> +		return;
> +
> +	qmi_param_free(param);
> +
> +error:
> +	CALLBACK_WITH_FAILURE(cb, cbd->data);
> +	l_free(cbd);
> +}
> +
>   static void get_card_status_cb(struct qmi_result *result, void *user_data)
>   {
>   	struct ofono_sim *sim = user_data;
> @@ -987,6 +1056,7 @@ static const struct ofono_sim_driver driver = {
>   	.query_pin_retries	= qmi_query_pin_retries,
>   	.send_passwd		= qmi_pin_send,
>   	.query_facility_lock	= qmi_query_locked,
> +	.lock			= qmi_lock,
>   };
>   
>   OFONO_ATOM_DRIVER_BUILTIN(sim, qmimodem, &driver)
> diff --git a/drivers/qmimodem/uim.h b/drivers/qmimodem/uim.h
> index 439e25b..602ec0f 100644
> --- a/drivers/qmimodem/uim.h
> +++ b/drivers/qmimodem/uim.h
> @@ -11,6 +11,7 @@
>   #define QMI_UIM_WRITE_RECORD		35	/* Write a record */
>   #define QMI_UIM_GET_FILE_ATTRIBUTES	36	/* Get file attributes */
>   
> +#define QMI_UIM_ENABLE_PIN		37	/* Set PIN protection */
>   #define QMI_UIM_VERIFY_PIN		38	/* Verify PIN */
>   
>   #define QMI_UIM_EVENT_REGISTRATION	46	/* Register for indications */

Otherwise, LGTM.

Regards,
-Denis
Ivaylo Dimitrov Nov. 18, 2024, 5:09 p.m. UTC | #3 
Hi Denis,


On 18.11.24 г. 18:54 ч., Denis Kenzior wrote:
> Hi Ivo,
> 
> On 11/17/24 6:03 AM, Ivaylo Dimitrov wrote:
>> ---
>>   drivers/qmimodem/sim.c | 70 
>> ++++++++++++++++++++++++++++++++++++++++++++++++++
>>   drivers/qmimodem/uim.h |  1 +
>>   2 files changed, 71 insertions(+)
> 
> Looks like this patch is on top of the query_facility_lock.  Please send 
> these as a series in the future, otherwise the CI gets confused.
> 

I was thinking it will take me a while to implement $subject 
functionality, it turned out to be easy.

As query_facility_lock needs changes anyway, I'll resend as series.

>>
>> diff --git a/drivers/qmimodem/sim.c b/drivers/qmimodem/sim.c
>> index b1d8f22..65c0fa6 100644
>> --- a/drivers/qmimodem/sim.c
>> +++ b/drivers/qmimodem/sim.c
>> @@ -844,6 +844,75 @@ static void qmi_query_locked(struct ofono_sim *sim,
>>       l_free(cbd);
>>   }
>> +static void qmi_lock(struct ofono_sim *sim,
>> +        enum ofono_sim_password_type passwd_type,
>> +        int enable, const char *passwd,
>> +        ofono_sim_lock_unlock_cb_t cb, void *user_data)
>> +{
>> +    struct sim_data *data = ofono_sim_get_data(sim);
>> +    struct cb_data *cbd = cb_data_new(cb, user_data);
>> +    int passwd_len;
>> +    uint16_t info_len;
>> +    uint8_t pin_id;
>> +    struct qmi_param *param;
>> +    uint8_t session[2];
>> +    struct {
>> +        uint8_t id;
>> +        uint8_t enabled;
>> +        uint8_t length;
>> +        uint8_t pin[0];
>> +    } __attribute__((__packed__)) *info;
>> +
>> +    DBG("");
>> +
>> +    switch (passwd_type) {
>> +    case OFONO_SIM_PASSWORD_SIM_PIN:
>> +        pin_id = 0x01;
>> +        break;
>> +    case OFONO_SIM_PASSWORD_SIM_PIN2:
>> +        pin_id = 0x02;
>> +        break;
>> +    default:
>> +        goto error;
>> +    }
>> +
>> +    if (!passwd)
>> +        goto error;
>> +
>> +    passwd_len = strlen(passwd);
>> +
>> +    if (passwd_len <= 0 || passwd_len > 0xFF)
> 
> strlen returns a size_t, so checking < 0 doesn't really make sense.  
> Also, the core already performs validation of the pin using 
> 'is_valid_pin'.  So I'm not really sure any of this validation is needed?
> 

I borrowed the code from qmi_pin_send(), shall I create another patch 
removing the checks there too?

>> +        goto error;
>> +
>> +    param = qmi_param_new();
>> +
>> +    /* info */
>> +    info_len = sizeof(*info) + passwd_len;
>> +    info = alloca(info_len);
>> +    info->id = pin_id;
>> +    info->enabled = enable ? 0x01 : 0x00;
>> +    info->length = (uint8_t) passwd_len;
>> +    memcpy(info->pin, passwd, passwd_len);
>> +
>> +    qmi_param_append(param, QMI_UIM_PARAM_MESSAGE_INFO, info_len, info);
>> +
>> +    /* session */
>> +    session[0] = 0x6; /* card on slot 1 */
>> +    session[1] = 0x0;
> 
> Hmm, can we get some #defines for these magic numbers? Or static const 
> variable declaration?
> 

sure, will create a separate patch (introducing some #defines) and 
fixing qmi_pin_send() as well.

>> +    qmi_param_append(param, QMI_UIM_PARAM_MESSAGE_SESSION_INFO,
>> +                    sizeof(session), session);
>> +
>> +    if (qmi_service_send(data->uim, QMI_UIM_ENABLE_PIN, param,
>> +                    pin_send_cb, cbd, cb_data_unref) > 0)
>> +        return;
>> +
>> +    qmi_param_free(param);
>> +
>> +error:
>> +    CALLBACK_WITH_FAILURE(cb, cbd->data);
>> +    l_free(cbd);
>> +}
>> +
>>   static void get_card_status_cb(struct qmi_result *result, void 
>> *user_data)
>>   {
>>       struct ofono_sim *sim = user_data;
>> @@ -987,6 +1056,7 @@ static const struct ofono_sim_driver driver = {
>>       .query_pin_retries    = qmi_query_pin_retries,
>>       .send_passwd        = qmi_pin_send,
>>       .query_facility_lock    = qmi_query_locked,
>> +    .lock            = qmi_lock,
>>   };
>>   OFONO_ATOM_DRIVER_BUILTIN(sim, qmimodem, &driver)
>> diff --git a/drivers/qmimodem/uim.h b/drivers/qmimodem/uim.h
>> index 439e25b..602ec0f 100644
>> --- a/drivers/qmimodem/uim.h
>> +++ b/drivers/qmimodem/uim.h
>> @@ -11,6 +11,7 @@
>>   #define QMI_UIM_WRITE_RECORD        35    /* Write a record */
>>   #define QMI_UIM_GET_FILE_ATTRIBUTES    36    /* Get file attributes */
>> +#define QMI_UIM_ENABLE_PIN        37    /* Set PIN protection */
>>   #define QMI_UIM_VERIFY_PIN        38    /* Verify PIN */
>>   #define QMI_UIM_EVENT_REGISTRATION    46    /* Register for 
>> indications */
> 
> Otherwise, LGTM.
> 
> Regards,
> -Denis

Thanks,
Ivo
Denis Kenzior Nov. 18, 2024, 5:32 p.m. UTC | #4 
Hi Ivo,

<snip>

 >>> +    passwd_len = strlen(passwd);
>>> +
>>> +    if (passwd_len <= 0 || passwd_len > 0xFF)
>>
>> strlen returns a size_t, so checking < 0 doesn't really make sense. Also, the 
>> core already performs validation of the pin using 'is_valid_pin'.  So I'm not 
>> really sure any of this validation is needed?
>>
> 
> I borrowed the code from qmi_pin_send(), shall I create another patch removing 
> the checks there too?

Yeah I think that would be okay.  Generally, the driver should trust that the 
core is performing the needed checks.  The driver should perform any additional 
checks that might be needed specifically for that driver, but these should be rare.

Regards,
-Denis
diff mbox series

Patch

diff --git a/drivers/qmimodem/sim.c b/drivers/qmimodem/sim.c
index b1d8f22..65c0fa6 100644
--- a/drivers/qmimodem/sim.c
+++ b/drivers/qmimodem/sim.c
@@ -844,6 +844,75 @@  static void qmi_query_locked(struct ofono_sim *sim,
 	l_free(cbd);
 }
 
+static void qmi_lock(struct ofono_sim *sim,
+		enum ofono_sim_password_type passwd_type,
+		int enable, const char *passwd,
+		ofono_sim_lock_unlock_cb_t cb, void *user_data)
+{
+	struct sim_data *data = ofono_sim_get_data(sim);
+	struct cb_data *cbd = cb_data_new(cb, user_data);
+	int passwd_len;
+	uint16_t info_len;
+	uint8_t pin_id;
+	struct qmi_param *param;
+	uint8_t session[2];
+	struct {
+		uint8_t id;
+		uint8_t enabled;
+		uint8_t length;
+		uint8_t pin[0];
+	} __attribute__((__packed__)) *info;
+
+	DBG("");
+
+	switch (passwd_type) {
+	case OFONO_SIM_PASSWORD_SIM_PIN:
+		pin_id = 0x01;
+		break;
+	case OFONO_SIM_PASSWORD_SIM_PIN2:
+		pin_id = 0x02;
+		break;
+	default:
+		goto error;
+	}
+
+	if (!passwd)
+		goto error;
+
+	passwd_len = strlen(passwd);
+
+	if (passwd_len <= 0 || passwd_len > 0xFF)
+		goto error;
+
+	param = qmi_param_new();
+
+	/* info */
+	info_len = sizeof(*info) + passwd_len;
+	info = alloca(info_len);
+	info->id = pin_id;
+	info->enabled = enable ? 0x01 : 0x00;
+	info->length = (uint8_t) passwd_len;
+	memcpy(info->pin, passwd, passwd_len);
+
+	qmi_param_append(param, QMI_UIM_PARAM_MESSAGE_INFO, info_len, info);
+
+	/* session */
+	session[0] = 0x6; /* card on slot 1 */
+	session[1] = 0x0;
+	qmi_param_append(param, QMI_UIM_PARAM_MESSAGE_SESSION_INFO,
+					sizeof(session), session);
+
+	if (qmi_service_send(data->uim, QMI_UIM_ENABLE_PIN, param,
+					pin_send_cb, cbd, cb_data_unref) > 0)
+		return;
+
+	qmi_param_free(param);
+
+error:
+	CALLBACK_WITH_FAILURE(cb, cbd->data);
+	l_free(cbd);
+}
+
 static void get_card_status_cb(struct qmi_result *result, void *user_data)
 {
 	struct ofono_sim *sim = user_data;
@@ -987,6 +1056,7 @@  static const struct ofono_sim_driver driver = {
 	.query_pin_retries	= qmi_query_pin_retries,
 	.send_passwd		= qmi_pin_send,
 	.query_facility_lock	= qmi_query_locked,
+	.lock			= qmi_lock,
 };
 
 OFONO_ATOM_DRIVER_BUILTIN(sim, qmimodem, &driver)
diff --git a/drivers/qmimodem/uim.h b/drivers/qmimodem/uim.h
index 439e25b..602ec0f 100644
--- a/drivers/qmimodem/uim.h
+++ b/drivers/qmimodem/uim.h
@@ -11,6 +11,7 @@ 
 #define QMI_UIM_WRITE_RECORD		35	/* Write a record */
 #define QMI_UIM_GET_FILE_ATTRIBUTES	36	/* Get file attributes */
 
+#define QMI_UIM_ENABLE_PIN		37	/* Set PIN protection */
 #define QMI_UIM_VERIFY_PIN		38	/* Verify PIN */
 
 #define QMI_UIM_EVENT_REGISTRATION	46	/* Register for indications */