Addditional classmark check against A5/X support

Signed-off-by: Max Suraev <Max.Suraev@fairwaves.co>
---
 include/osmocom/gsm/gsm_utils.h | 28 ++++++++++++++++++++++++++--
 1 file changed, 26 insertions(+), 2 deletions(-)

Hi,

>  /* According to GSM 04.08 Chapter 10.5.1.6 */
> -static inline int ms_cm2_a5n_support(uint8_t *cm2, int n) {
> +static inline int ms_cm2_a5n_support(uint8_t *cm2, unsigned n) {
>         switch (n) {
>                 case 0: return 1;
>                 case 1: return (cm2[0] & (1<<3)) ? 0 : 1;
>                 case 2: return (cm2[2] & (1<<0)) ? 1 : 0;
>                 case 3: return (cm2[2] & (1<<1)) ? 1 : 0;
>                 default:
> -                       return 0;
> +                       return (n > 7) ? 0 : -1;
>         }
>  }

Why this change ?  I mean, you'd now have to go over __every_ use of
that function in all projects and make sure it's not used in something
like :

if (!ms_cm2_a5n_support(cm2, n)) {
   error
}

Because an invalid n is now going to return something != 0 ...


> +/*! \brief Check whether MS supports given cipher
> + *  \param[in] cm Classmark data transmitted by MS, cannot be NULL
> + *  \param[in] n Cipher number - A5/n
> + *  \returns 1 if supported, 0 if unsupported, -1 on failures
> + *
> + * Implementation based on specifications from GSM 04.08
> + * parts 10.5.1.6 and 10.5.1.7.
> + */
> +static inline int ms_a5n_support(uint8_t *cm, unsigned n) {
> +    return ((n < 4) ? ms_cm2_a5n_support(cm, n) : ms_cm3_a5n_support(cm, n));
> +}
> +

Huh ... so the called has to know whether to give CM2 or CM3 ... you
might as well not have this method at all then and just require it to
call the right method.


Cheers,

   Sylvain

16.06.2014 23:04, Sylvain Munaut пишет:
>
>> -                       return 0;
>> +                       return (n > 7) ? 0 : -1;
>>         }
>>  }
> 
> Why this change ?  I mean, you'd now have to go over __every_ use of
> that function in all projects and make sure it's not used in something
> like :
> 
> if (!ms_cm2_a5n_support(cm2, n)) {
>    error
> }
> 
> Because an invalid n is now going to return something != 0 ...
> 

I thought it's better to expose error case explicitly.


>> +static inline int ms_a5n_support(uint8_t *cm, unsigned n) {
>> +    return ((n < 4) ? ms_cm2_a5n_support(cm, n) : ms_cm3_a5n_support(cm, n));
>> +}
>> +
> 
> Huh ... so the called has to know whether to give CM2 or CM3 ... you
> might as well not have this method at all then and just require it to
> call the right method.
> 

I'm not sure what you mean in here - we call this function with some classmark
(either 2 or 3) and number from a5/n to check if this cipher support is indicated in
the classmark. Anyway - just a little convenience wrapper.

On Tue, Jun 17, 2014 at 10:56 AM, ☎ <Max.Suraev@fairwaves.co> wrote:
> 16.06.2014 23:04, Sylvain Munaut пишет:
>>
>>> -                       return 0;
>>> +                       return (n > 7) ? 0 : -1;
>>>         }
>>>  }
>>
>> Why this change ?  I mean, you'd now have to go over __every_ use of
>> that function in all projects and make sure it's not used in something
>> like :
>>
>> if (!ms_cm2_a5n_support(cm2, n)) {
>>    error
>> }
>>
>> Because an invalid n is now going to return something != 0 ...
>>
>
> I thought it's better to expose error case explicitly.

In this case you can't change the API. Because the case I showed above
is _exactly_ how it's used currently so if someone has a new
libosmocore and an old openbsc, it will build fine but not do the
right thing.

And in either case a return value of 0 for n>7 is certainly not wrong
since you can't possibly support an non-existent cipher.



>>> +static inline int ms_a5n_support(uint8_t *cm, unsigned n) {
>>> +    return ((n < 4) ? ms_cm2_a5n_support(cm, n) : ms_cm3_a5n_support(cm, n));
>>> +}
>>> +
>>
>> Huh ... so the called has to know whether to give CM2 or CM3 ... you
>> might as well not have this method at all then and just require it to
>> call the right method.
>>
>
> I'm not sure what you mean in here - we call this function with some classmark
> (either 2 or 3) and number from a5/n to check if this cipher support is indicated in
> the classmark. Anyway - just a little convenience wrapper.

What I mean is that it's not "either 2 or 3"  it _needs_ to be CM2 for
a5/[1-3] and CM3 for a5/[4-7] ... so that means that the caller _HAS_
to have the knowledge of which CM to check.

If you want it to make convenient, then I'd take _both_ CM2 and CM3 in
argument and choose the right one.

Possibly make it accept NULL for CM2/CM3 and if you need a missing
info to make the decision, return -EAGAIN.
And make proper doc for this behavior. Since this is a new function,
and this actually provides a useful info you can use error code for
this.


Cheers,

   Sylvain