Re: [PATCH v8 7/8] mbim: Add support for SIM I/O operations

On 12/12/25 16:02, Muhammad Asif wrote:
> Adds support for complete SIM I/O operations, with complete path
> resolution using sim_ef_db.
[...]
> --- a/drivers/mbimmodem/sim.c
> +++ b/drivers/mbimmodem/sim.c
> @@ -22,14 +22,39 @@
>   #include "drivers/mbimmodem/mbim-message.h"
>   #include "drivers/mbimmodem/mbimmodem.h"
>   
> +struct sim_app {
> +	enum mbim_app_type app_type;
> +	uint32_t aid_len;
> +	uint8_t *aid;
> +	char *label;
> +	uint32_t pin_key_references;
> +};
> +
>   struct sim_data {
>   	struct mbim_device *device;
> +	uint32_t app_count;
> +	uint32_t active_app;
> +	struct sim_app *apps;
>   	char *iccid;
>   	char *imsi;
>   	uint32_t last_pin_type;
>   	bool present : 1;
>   };
>   
> +static uint32_t mbim_file_structure_to_ofono(uint32_t file_structure)
> +{
> +	switch (file_structure) {
> +	case 1:
> +		return OFONO_SIM_FILE_STRUCTURE_TRANSPARENT;
> +	case 2:
> +		return OFONO_SIM_FILE_STRUCTURE_CYCLIC;
> +	case 3:
> +		return OFONO_SIM_FILE_STRUCTURE_FIXED;
> +	default:
> +		return OFONO_SIM_FILE_STRUCTURE_TRANSPARENT;
> +	}
> +}
> +
>   static void mbim_sim_state_changed(struct ofono_sim *sim, uint32_t ready_state)
>   {
>   	struct sim_data *sd = ofono_sim_get_data(sim);
> @@ -76,6 +101,136 @@ static void mbim_read_imsi(struct ofono_sim *sim,
>   	CALLBACK_WITH_SUCCESS(cb, sd->imsi, user_data);
>   }
>   
> +static void read_file_info_cb(struct mbim_message *message, void *user_data)
> +{
> +	struct cb_data *cbd = user_data;
> +	ofono_sim_file_info_cb_t cb = cbd->cb;
> +	unsigned char access[3] = {0x0f, 0xff, 0xff};
> +	uint32_t version, status_word1, status_word2, file_accessibility;
> +	uint32_t file_type, file_structure, file_item_count, file_item_size;
> +
> +	if (!mbim_message_get_error(message)) {
> +		mbim_message_get_arguments(message, "uuuuuuuu", &version, &status_word1,

This can fail, and the return value should be checked.


> +					&status_word2, &file_accessibility,
> +					&file_type, &file_structure,
> +					&file_item_count, &file_item_size);
> +
> +		CALLBACK_WITH_SUCCESS(cb, file_item_size,
> +					mbim_file_structure_to_ofono(file_structure),
> +					file_item_size * file_item_count,
> +					access, file_accessibility, cbd->data);
> +	} else {
> +		CALLBACK_WITH_FAILURE(cb, -1, -1, -1, NULL, -1, cbd->data);
> +	}
> +}
> +
> +static void mbim_read_file_info(struct ofono_sim *sim,
> +				int fileid, const unsigned char *path,
> +				unsigned int path_len,
> +				ofono_sim_file_info_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);
> +	struct mbim_message *message;
> +	uint8_t *file_id_buf;
> +	int file_id_len;
> +
> +	message = mbim_message_new(mbim_ms_uicc_low_level_access,
> +					MBIM_CID_MS_UICC_LOW_LEVEL_ACCESS_FILE_STATUS,
> +					MBIM_COMMAND_TYPE_QUERY);
> +
> +	file_id_buf = mbim_get_fileid(data->apps[data->active_app].app_type, fileid, &file_id_len);
> +	mbim_message_set_arguments(message, "uayay", 1,
> +					data->apps[data->active_app].aid_len,
> +					data->apps[data->active_app].aid,
> +					file_id_len, file_id_buf);
> +	l_free(file_id_buf);
> +
> +	mbim_device_send(data->device, SIM_GROUP, message, read_file_info_cb, cbd, l_free);
> +}
> +
> +static void read_file_cb(struct mbim_message *message, void *user_data)
> +{
> +	struct cb_data *cbd = user_data;
> +	ofono_sim_read_cb_t cb = cbd->cb;
> +	struct mbim_message_iter iter;
> +	uint32_t version, status_word1, status_word2, data_size;
> +	uint8_t *data = NULL;
> +	int i = 0;
> +
> +	if (!mbim_message_get_error(message)) {
> +		mbim_message_get_arguments(message, "uuuAy", &version,

This can fail, and the return value should be checked.

> +						&status_word1, &status_word2, &iter);
> +
> +		data_size = iter.n_elem;
> +		data = l_malloc(data_size);
> +		while (mbim_message_iter_next_entry(&iter, data + i))
> +			i++;
> +
> +		CALLBACK_WITH_SUCCESS(cb, data, data_size, cbd->data);
> +	} else {
> +		CALLBACK_WITH_FAILURE(cb, NULL, 0, cbd->data);
> +	}
> +
> +	/* The data gets copied by ofono's sim driver, so we can free it */
> +	if (data)
> +		l_free(data);
> +}
> +
> +static void mbim_read_file_transparent(struct ofono_sim *sim,
> +					int fileid, int start, int length,
> +					const unsigned char *path, unsigned int path_len,
> +					ofono_sim_read_cb_t cb, void *user_data)
> +{
> +	struct sim_data *sd = ofono_sim_get_data(sim);
> +	struct cb_data *cbd = cb_data_new(cb, user_data);
> +	struct mbim_message *message;
> +	uint8_t *file_id_buf;
> +	int file_id_len;
> +
> +	message = mbim_message_new(mbim_ms_uicc_low_level_access,
> +					MBIM_CID_MS_UICC_LOW_LEVEL_ACCESS_READ_BINARY,
> +					MBIM_COMMAND_TYPE_QUERY);
> +
> +	file_id_buf = mbim_get_fileid(sd->apps[sd->active_app].app_type, fileid, &file_id_len);
> +	mbim_message_set_arguments(message, "uayayuusay", 1,
> +					sd->apps[sd->active_app].aid_len,
> +					sd->apps[sd->active_app].aid,
> +					file_id_len, file_id_buf, start,
> +					length, "", 0, NULL);
> +	l_free(file_id_buf);
> +
> +	mbim_device_send(sd->device, SIM_GROUP, message,
> +				read_file_cb, cbd, l_free);
> +}
> +
> +static void mbim_read_file_fixed_cyclic(struct ofono_sim *sim,
> +					int fileid, int record, int length,
> +					const unsigned char *path, unsigned int path_len,
> +					ofono_sim_read_cb_t cb, void *data)
> +{
> +	struct sim_data *sd = ofono_sim_get_data(sim);
> +	struct cb_data *cbd = cb_data_new(cb, data);
> +	struct mbim_message *message;
> +	uint8_t *file_id_buf;
> +	int file_id_len;
> +
> +	message = mbim_message_new(mbim_ms_uicc_low_level_access,
> +					MBIM_CID_MS_UICC_LOW_LEVEL_ACCESS_READ_RECORD,
> +					MBIM_COMMAND_TYPE_QUERY);
> +
> +	file_id_buf = mbim_get_fileid(sd->apps[sd->active_app].app_type, fileid, &file_id_len);
> +	mbim_message_set_arguments(message, "uayayusay", 1,
> +					sd->apps[sd->active_app].aid_len,
> +					sd->apps[sd->active_app].aid,
> +					file_id_len, file_id_buf, record,
> +					"", 0, NULL);
> +	l_free(file_id_buf);
> +
> +	mbim_device_send(sd->device, SIM_GROUP, message,
> +				read_file_cb, cbd, l_free);
> +}
> +
>   static enum ofono_sim_password_type mbim_pin_type_to_sim_password(
>   							uint32_t pin_type)
>   {
> @@ -473,6 +628,47 @@ error:
>   	ofono_sim_remove(sim);
>   }
>   
> +static void mbim_sim_list_apps_cb(struct mbim_message *message,
> +					void *user)
> +{
> +	struct sim_data *data = ofono_sim_get_data(user);
> +	uint32_t version, count, active_app;
> +	uint32_t application_id, size, pin_key_references;
> +	char *aid_label;
> +	struct mbim_message_iter iter, aid_iter, pin_iter;
> +	int i = 0;
> +

For correctness, set data->app_count = 0 and data->apps = NULL; here 
instead of below in the mbim_message_get_arguments() error path.

Then you can just return in the case of errors in the next two function 
calls.


> +	if (mbim_message_get_error(message))
> +		return;
> +
> +	if (!mbim_message_get_arguments(message, "uuuua(uAysuAy)", &version,
> +					&count, &active_app, &size, &iter)) {
> +		data->apps = NULL;
> +		data->app_count = 0;
> +		return;
> +	}

Oh, and one more thing; you might want to verify that count is a 
reasonable value (and return if > $bignumber. I don't know how often you 
might get nonsense from the sim, but.. Likewise, sanity checking for 
active_app (active_app < app_acount).


> +
> +	data->apps = l_new(struct sim_app, count);
> +	data->active_app = active_app;
> +	data->app_count = count;
> +

Personally I'd use a for loop here for clarity, but that's really up to 
you. But if you stick with the while() loop, I would set i=0 here to 
make it clearer what's going on.

> +	while (mbim_message_iter_next_entry(&iter, &application_id, &aid_iter,
> +					&aid_label, &pin_key_references, &pin_iter)) {
 > +		int j = 0;> +
> +		data->apps[i].app_type = application_id;
> +		data->apps[i].aid_len = aid_iter.n_elem;
> +		data->apps[i].aid = l_malloc(aid_iter.n_elem);


In the mbim_sim_remove function below, you free apps[i].label. But here 
you're getting label from the message_iter, and also allocating 
apps[i].aid but never free it. That's a bit confusing.. Did you mean to 
g_strdup(aid_label), and also l_free(sd->apps[i].aid) in the remove 
function below?


> +
> +		while (mbim_message_iter_next_entry(&aid_iter,
> +							data->apps[i].aid + j))
> +			j++;
> +
> +		data->apps[i].label = aid_label;
> +		i++;
> +	}
> +}
> +
>   static int mbim_sim_probe(struct ofono_sim *sim, unsigned int vendor,
>   				void *data)
>   {
> @@ -480,6 +676,14 @@ static int mbim_sim_probe(struct ofono_sim *sim, unsigned int vendor,
>   	struct mbim_message *message;
>   	struct sim_data *sd;
>   
> +	message = mbim_message_new(mbim_ms_uicc_low_level_access,
> +					MBIM_CID_MS_UICC_LOW_LEVEL_ACCESS_APPLICATION_LIST,
> +					MBIM_COMMAND_TYPE_QUERY);

For consistency, I would recommend verifying that message != NULL here 
as is done later in the function. It's a probe function, I'm guessing 
someone decided that it's a really good time to bail out if memory is tight.



> +	mbim_message_set_arguments(message, "");
> +
> +	mbim_device_send(device, SIM_GROUP, message,
> +			mbim_sim_list_apps_cb, sim, NULL);
> +
>   	message = mbim_message_new(mbim_uuid_basic_connect,
>   					MBIM_CID_SUBSCRIBER_READY_STATUS,
>   					MBIM_COMMAND_TYPE_QUERY);
> @@ -514,12 +718,22 @@ static void mbim_sim_remove(struct ofono_sim *sim)
>   
>   	l_free(sd->iccid);
>   	l_free(sd->imsi);
> +
> +	for (int i = 0; i < sd->app_count; i++)
> +		l_free(sd->apps[i].label);
> +
> +	l_free(sd->apps);
> +
>   	l_free(sd);
>   }
>   
>   static const struct ofono_sim_driver driver = {
>   	.probe			= mbim_sim_probe,
>   	.remove			= mbim_sim_remove,
> +	.read_file_info		= mbim_read_file_info,
> +	.read_file_transparent	= mbim_read_file_transparent,
> +	.read_file_cyclic	= mbim_read_file_fixed_cyclic,
> +	.read_file_linear	= mbim_read_file_fixed_cyclic,
>   	.read_imsi		= mbim_read_imsi,
>   	.query_passwd_state	= mbim_pin_query,
>   	.query_pin_retries	= mbim_pin_retries_query,
> diff --git a/drivers/mbimmodem/util.c b/drivers/mbimmodem/util.c
> index 4a3d9627..3280c863 100644
> --- a/drivers/mbimmodem/util.c
> +++ b/drivers/mbimmodem/util.c
> @@ -9,6 +9,7 @@
>   #include <stdbool.h>
>   
>   #include "src/common.h"
> +#include "simutil.h"
>   #include "mbim.h"
>   #include "util.h"
>   
> @@ -37,3 +38,33 @@ int mbim_data_class_to_tech(uint32_t n)
>   
>   	return -1;
>   }
> +
> +uint8_t *mbim_get_fileid(enum mbim_app_type app_type, uint32_t fileid, int *file_id_len)

Maybe mbim_get_fileid_path(), similar to isimodem's naming? Or 
mbim_get_fileid_path_new(), to make it clear that the returned memory 
should be freed by the caller?


> +{
> +	uint8_t parent_path[6] = {0};
> +	int fileid_len = 0;
> +	uint8_t *full_path;
> +
> +	if (app_type == MBIM_APP_USIM || app_type == MBIM_APP_ISIM)
> +		sim_ef_db_get_path_3g(fileid, parent_path);
> +	else
> +		sim_ef_db_get_path_2g(fileid, parent_path);


We should probably make sure sim_ef_db_get_path*() calls don't fail; if 
the result is < 2, I'm not sure what you should do.

> +
> +	/* Copy the parent_path up until the first pair of 0's */
> +	for (int i = 0; i < 6; i += 2) {
> +		if (i != 0 && parent_path[i] == 0x00

So you're always skipping over the first iteration of this loop? In that 
case, maybe start at 2 for clarity?  Eg,
		len = sim_ef_db_get_path_2g(...);

	if (len < 2 || len > 6) {
		/* set *file_id_len = -1 and return NULL?  set *file_id_len = 2, 
allocate an empty 2 byte full_path, fill it with fileid, and return it? */
	}

	for (i = 2; i < len; i += 2) {
		if (parent_path[i] == 0x0 && parent_path[i+1] == 0x0) {
			fileid_len = i;
			break;
		}
	}

> +			&& parent_path[i + 1] == 0x00) {
> +			fileid_len = i;
> +			break;
> +		}
> +	}
> +
> +	full_path = l_malloc(fileid_len + 2);
> +	memcpy(full_path, parent_path, fileid_len);
> +
> +	full_path[fileid_len] = (fileid >> 8) & 0xFF;
> +	full_path[fileid_len + 1] = fileid & 0xFF;
> +	*file_id_len = fileid_len + 2;
> +
> +	return full_path;
> +}
> diff --git a/drivers/mbimmodem/util.h b/drivers/mbimmodem/util.h
> index 2172cd9f..55669450 100644
> --- a/drivers/mbimmodem/util.h
> +++ b/drivers/mbimmodem/util.h
> @@ -43,3 +43,4 @@ static inline struct cb_data *cb_data_new(void *cb, void *data)
>   	} while (0)
>   
>   int mbim_data_class_to_tech(uint32_t n);
> +uint8_t *mbim_get_fileid(enum mbim_app_type app_type, uint32_t fileid, int *file_id_len);