[PATCH] Hardening PORTCFG parse reply

[Carlo Lobrano]

Hi Dan,

I'm using a standard Ubuntu 16.06. I tested with both GUI interface
and rtcwake, but the "problem" is that the system I was working with
detaches the modem when it goes in stand by, so there's a full
de-enumeration/re-enumeration of the ports when passing from S0 to S3
and back. When the system wakes up I saw both messages for disposing
the modem and the ones that probe the "new" modem connected and they
goes to the same ports.

When I debugged the problem I saw that the parser of PORTCFG response
(mm-common-telit.c:cache_port_mode) receives an empty string as reply,
so that the statement at line 138

    if (!g_regex_match_full (r, reply, strlen (reply), 0, 0,
&match_info, &error))

returns FALSE

I do not know how, but it seemed that the logic behind the AT
communication in MM mistakenly gets this message as reply of my AT
command, while the correct reply from the modem is shown in the logs,
so it is not modem's fault.

The best solution would be to provide for each AT command the expected
response's prefix (PORTCFG in this case) to be checked in order to be
sure that the reply is the one expected, but this change is really too
much for this problem (also because I never observed other cases where
this is really necessary)

However, I saw that this patch does not apply to the current master
branch, where the modified functions are in a different file. If you
think that this change could be applied, I will provide the updated
patch.




On 10 June 2016 at 19:27, Dan Williams <dcbw at redhat.com> wrote:
> On Wed, 2016-06-01 at 14:51 +0200, Carlo Lobrano wrote:
>> A brief explaination of the issue that this patch solves.
>> I observed a probing failure at resume from memory (S3), caused by a
>> missing or wrong reply to #PORTCFG? AT command sent during port
>> probing.
>> This error is probably due to the overposition of multiple messages
>> and replies,
>> since at the same time the ModemManager is trying and close the last
>> connection,
>> and probing a new modem device, and the PORTCFG parser receive an
>> empty string to
>> parse, causing the failure.
>> Therefore the fix consists in the hardening of the reply to this
>> command and
>> in providing a default port configuration in case of the retries are
>> not enough.
>
> Hmm, this seems like a hackish workaround...  What suspend/resume
> manager are you using?  Ideally MM would either block suspend until it
> had cleaned up and disposed the modem, or on resume it would just
> force-close everything, dispose the modem, and reprobe.
>
> Dan
>
>> ---
>>
>> - Increase PORTCFG retries during initialization from 3 to 5 retries.
>> - Provide a default PORTCFG configuration in case of missing or wrong
>> reply.
>> ---
>>  plugins/telit/mm-plugin-telit.c | 44 +++++++++++++++++++++++++++--
>> ------------
>>  1 file changed, 29 insertions(+), 15 deletions(-)
>>
>> diff --git a/plugins/telit/mm-plugin-telit.c b/plugins/telit/mm-
>> plugin-telit.c
>> index a3620e7..19eed8f 100644
>> --- a/plugins/telit/mm-plugin-telit.c
>> +++ b/plugins/telit/mm-plugin-telit.c
>> @@ -165,8 +165,10 @@ cache_port_mode (MMDevice *device,
>>      r = g_regex_new ("#PORTCFG:\\s*(\\d+),(\\d+)", flags, 0, NULL);
>>      g_assert (r != NULL);
>>
>> -    if (!g_regex_match_full (r, reply, strlen (reply), 0, 0,
>> &match_info, &error))
>> +    if (!g_regex_match_full (r, reply, strlen (reply), 0, 0,
>> &match_info, &error)) {
>> +        mm_dbg ("Unexpected PORTCFG reply '%s'. Could not parse
>> it.", reply);
>>          goto out;
>> +    }
>>
>>      if (!mm_get_uint_from_match_info (match_info, 2,
>> &portcfg_current)) {
>>          mm_dbg ("telit: unrecognized #PORTCFG <active> value");
>> @@ -201,6 +203,7 @@ cache_port_mode (MMDevice *device,
>>          break;
>>      default:
>>          /* portcfg value not supported */
>> +        mm_err ("Invalid portcfg value from reply '%s'", reply);
>>          goto out;
>>      }
>>      ret = TRUE;
>> @@ -222,29 +225,40 @@ getportcfg_ready (MMPortSerialAt *port,
>>  {
>>      const gchar *response;
>>      GError *error = NULL;
>> +    MMDevice *device;
>>
>> +    device = mm_port_probe_peek_device (ctx->probe);
>>      response = mm_port_serial_at_command_finish (port, res, &error);
>> -    if (error) {
>> -        mm_dbg ("telit: couldn't get port mode: '%s'",
>> -                error->message);
>>
>> -        /* If ERROR or COMMAND NOT SUPPORT occur then do not retry
>> the
>> -         * command.
>> -         */
>> -        if (g_error_matches (error,
>> -                             MM_MOBILE_EQUIPMENT_ERROR,
>> -                             MM_MOBILE_EQUIPMENT_ERROR_UNKNOWN))
>> -            ctx->getportcfg_done = TRUE;
>> +    if (error) {
>> +        mm_dbg ("telit: couldn't get port mode: '%s' (retries left
>> %d)",
>> +                error->message,
>> +                ctx->getportcfg_retries);
>> +
>> +        if (ctx->getportcfg_retries == 0) {
>> +            mm_dbg ("telit: getportcfg last retry failed. Try and
>> setting default port config.");
>> +            if (g_object_get_data (G_OBJECT (device),
>> TAG_GETPORTCFG_SUPPORTED) == NULL) {
>> +                mm_dbg ("telit: Defaulting to 00 and 06 for modem
>> and aux");
>> +                g_object_set_data (G_OBJECT (device),
>> TAG_TELIT_MODEM_PORT, "00");
>> +                g_object_set_data (G_OBJECT (device),
>> TAG_TELIT_AUX_PORT, "06");
>> +                g_object_set_data (G_OBJECT (device),
>> TAG_GETPORTCFG_SUPPORTED, GUINT_TO_POINTER (TRUE));
>> +                ctx->getportcfg_done = TRUE;
>> +            }
>> +        }
>>      } else {
>> -        MMDevice *device;
>> -        device = mm_port_probe_peek_device (ctx->probe);
>> -
>>          /* Results are cached in the parent device object */
>>          if (g_object_get_data (G_OBJECT (device),
>> TAG_GETPORTCFG_SUPPORTED) == NULL) {
>>              mm_dbg ("telit: retrieving port mode layout");
>>              if (cache_port_mode (device, response)) {
>>                  g_object_set_data (G_OBJECT (device),
>> TAG_GETPORTCFG_SUPPORTED, GUINT_TO_POINTER (TRUE));
>>                  ctx->getportcfg_done = TRUE;
>> +            } else {
>> +                mm_dbg ("telit: Error in cache port mode");
>> +                mm_dbg ("telit: Defaulting to 00 and 06 for modem
>> and aux");
>> +                g_object_set_data (G_OBJECT (device),
>> TAG_TELIT_MODEM_PORT, "00");
>> +                g_object_set_data (G_OBJECT (device),
>> TAG_TELIT_AUX_PORT, "06");
>> +                g_object_set_data (G_OBJECT (device),
>> TAG_GETPORTCFG_SUPPORTED, GUINT_TO_POINTER (TRUE));
>> +                ctx->getportcfg_done = TRUE;
>>              }
>>          }
>>
>> @@ -334,7 +348,7 @@ telit_custom_init (MMPortProbe *probe,
>>      ctx->port = g_object_ref (port);
>>      ctx->cancellable = cancellable ? g_object_ref (cancellable) :
>> NULL;
>>      ctx->getportcfg_done = FALSE;
>> -    ctx->getportcfg_retries = 3;
>> +    ctx->getportcfg_retries = 5;
>>
>>      /* If the device is tagged for supporting #PORTCFG do the custom
>> init */
>>      if (g_udev_device_get_property_as_boolean (udevDevice,
>> "ID_MM_TELIT_PORTS_TAGGED")) {