Hi Harald,
I have a stack corruption due the above method and here is my analysis of the problem...
set_system_infos is having a u_int8_t array with 23 bytes on the stack and is asking to generate system infos into this array...
Now what happens is: 1.) some system information types structs are already bigger than the 23 bytes... 2.) this does not take the rest octets into account..
I would like to fix it like this: 1.) Turn bitvec_spare_padding to return void 2.) In the rest_octets_siX method return the bit_vec.data_len 3.) Change the generate_siX to return the sizeof the struct + return value of the rest_octets_siX instead of the fixed MACBLOCK_LEN (23) 4.) always use this rc value instead of the size of the buffer... (due to 1. of the above we set truncated values as well)
do you have a better idea? would you just increase the buffer size?
z.
Hi Zecke,
On Thu, Dec 31, 2009 at 06:12:31AM +0100, Holger Freyther wrote:
I have a stack corruption due the above method and here is my analysis of the problem...
thanks for your analysis.
set_system_infos is having a u_int8_t array with 23 bytes on the stack and is asking to generate system infos into this array...
Now what happens is: 1.) some system information types structs are already bigger than the 23 bytes...
why are they? How can that be? How can a SI message be larger than the physical limitation of the MAC-Block? This sounds like the root cause of the problem to me.
I would like to fix it like this: 1.) Turn bitvec_spare_padding to return void
makes sense. after all, the caller already specifies the number of bits up to which he wants the data to be padded.
2.) In the rest_octets_siX method return the bit_vec.data_len
makes also sense to me.
3.) Change the generate_siX to return the sizeof the struct + return value of the rest_octets_siX instead of the fixed MACBLOCK_LEN (23) 4.) always use this rc value instead of the size of the buffer... (due to 1. of the above we set truncated values as well)
also fine with me.
do you have a better idea? would you just increase the buffer size?
as indicated above, the root cause might well be something else. Nevertheless, your suggested improvements are valid and correct.
On Thursday 31 December 2009 11:23:00 Harald Welte wrote:
Hi Zecke,
Now what happens is: 1.) some system information types structs are already bigger than the 23 bytes...
why are they? How can that be? How can a SI message be larger than the physical limitation of the MAC-Block? This sounds like the root cause of the problem to me.
This was bullshit...
Here is the root cause:
For SI5 and SI6 we have to deal with the BS11 of having left the length field out... What we are doing is:
char output[23]; if (is_nano_bts) { *output = len; ++output; }
si6 = (struct si6*) output; memset(si6, padding, 23);
And one thing I have found as well, but it seems more like I'm wrong. All data_len of the bitvector are one too big? Is that done on purpose?
Patch 0001 and 0003 are of cosmetic nature, 0002 and 0004 seem to fix the stack corruption my system is seeing.
regards holger
hi Zecke,
On Wed, Jan 06, 2010 at 07:59:21AM +0100, Holger Freyther wrote:
On Thursday 31 December 2009 11:23:00 Harald Welte wrote:
Hi Zecke,
Now what happens is: 1.) some system information types structs are already bigger than the 23 bytes...
why are they? How can that be? How can a SI message be larger than the physical limitation of the MAC-Block? This sounds like the root cause of the problem to me.
This was bullshit...
Here is the root cause:
For SI5 and SI6 we have to deal with the BS11 of having left the length field out... What we are doing is:
char output[23]; if (is_nano_bts) { *output = len; ++output; }
si6 = (struct si6*) output; memset(si6, padding, 23);
And one thing I have found as well, but it seems more like I'm wrong. All data_len of the bitvector are one too big? Is that done on purpose?
no, that is an artefact from the 04.08 spec always claiming that an IE has a certain length, but then icnluding the TAG, despite never sending the TAG in front of the IE on the actual air interface.
Patch 0001 and 0003 are of cosmetic nature, 0002 and 0004 seem to fix the stack corruption my system is seeing.
Please apply them, they're fine. In addition, we should probably remove the ++output from the generate_si, and have the RSL layer deal with it. Specifically: * include the system_information_type_header in SI5 and SI6 data structures * make sure the buffers passed to generate_si are large enough to include the l2_plen, maybe even check at runtime to be sure * in the RSL layer (rsl_sacch_filling()), check the trx->bts->type and either drop the length (BS11) or keep it (all other BTS)
Thanks in advance, Harald
-
Harald Welte -
Holger Freyther