Re: [patch] max17042_battery: fix a couple buffer overflows
On Thu, Mar 15, 2012 at 10:20:34AM -0700, Dirk Brandewie wrote: > On 03/15/2012 04:37 AM, Dan Carpenter wrote: > >There are a couple issues here caused by confusion between sizeof() > >and ARRAY_SIZE(). "table_size" should be the number of elements, but we > >should allocate it with kcalloc() so that we allocate the correct number > >of bytes. > > > >In max17042_init_model() we don't allocate enough space so we go past > >the end of the array in max17042_read_model_data() and > >max17042_model_data_compare(). > > > >In max17042_verify_model_lock() we allocate the right amount of space > >but we call max17042_read_model_data() with the wrong number of elements > >and also in the for loop we go past the end of the array. > > > >Signed-off-by: Dan Carpenter > Acked-by: Dirk Brandewie > > Typo in first ack :-( Applied, thanks! :-) -- Anton Vorontsov Email: cbouatmai...@gmail.com ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
[patch] max17042_battery: fix a couple buffer overflows
There are a couple issues here caused by confusion between sizeof() and ARRAY_SIZE(). "table_size" should be the number of elements, but we should allocate it with kcalloc() so that we allocate the correct number of bytes. In max17042_init_model() we don't allocate enough space so we go past the end of the array in max17042_read_model_data() and max17042_model_data_compare(). In max17042_verify_model_lock() we allocate the right amount of space but we call max17042_read_model_data() with the wrong number of elements and also in the for loop we go past the end of the array. Signed-off-by: Dan Carpenter --- I don't have this hardware. The original code is clearly buggy, but I can't test that my fix is correct. Please review carefully. diff --git a/drivers/power/max17042_battery.c b/drivers/power/max17042_battery.c index e36763a..f8cd48c 100644 --- a/drivers/power/max17042_battery.c +++ b/drivers/power/max17042_battery.c @@ -328,11 +328,10 @@ static inline int max17042_model_data_compare(struct max17042_chip *chip, static int max17042_init_model(struct max17042_chip *chip) { int ret; - int table_size = - sizeof(chip->pdata->config_data->cell_char_tbl)/sizeof(u16); + int table_size = ARRAY_SIZE(chip->pdata->config_data->cell_char_tbl); u16 *temp_data; - temp_data = kzalloc(table_size, GFP_KERNEL); + temp_data = kcalloc(table_size, sizeof(*temp_data), GFP_KERNEL); if (!temp_data) return -ENOMEM; @@ -357,12 +356,11 @@ static int max17042_init_model(struct max17042_chip *chip) static int max17042_verify_model_lock(struct max17042_chip *chip) { int i; - int table_size = - sizeof(chip->pdata->config_data->cell_char_tbl); + int table_size = ARRAY_SIZE(chip->pdata->config_data->cell_char_tbl); u16 *temp_data; int ret = 0; - temp_data = kzalloc(table_size, GFP_KERNEL); + temp_data = kcalloc(table_size, sizeof(*temp_data), GFP_KERNEL); if (!temp_data) return -ENOMEM; ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [patch] max17042_battery: fix a couple buffer overflows
On 03/15/2012 04:37 AM, Dan Carpenter wrote: There are a couple issues here caused by confusion between sizeof() and ARRAY_SIZE(). "table_size" should be the number of elements, but we should allocate it with kcalloc() so that we allocate the correct number of bytes. In max17042_init_model() we don't allocate enough space so we go past the end of the array in max17042_read_model_data() and max17042_model_data_compare(). In max17042_verify_model_lock() we allocate the right amount of space but we call max17042_read_model_data() with the wrong number of elements and also in the for loop we go past the end of the array. Signed-off-by: Dan Carpenter Acked-by: Dirk Brandewie Typo in first ack :-( --- I don't have this hardware. The original code is clearly buggy, but I can't test that my fix is correct. Please review carefully. diff --git a/drivers/power/max17042_battery.c b/drivers/power/max17042_battery.c index e36763a..f8cd48c 100644 --- a/drivers/power/max17042_battery.c +++ b/drivers/power/max17042_battery.c @@ -328,11 +328,10 @@ static inline int max17042_model_data_compare(struct max17042_chip *chip, static int max17042_init_model(struct max17042_chip *chip) { int ret; - int table_size = - sizeof(chip->pdata->config_data->cell_char_tbl)/sizeof(u16); + int table_size = ARRAY_SIZE(chip->pdata->config_data->cell_char_tbl); u16 *temp_data; - temp_data = kzalloc(table_size, GFP_KERNEL); + temp_data = kcalloc(table_size, sizeof(*temp_data), GFP_KERNEL); if (!temp_data) return -ENOMEM; @@ -357,12 +356,11 @@ static int max17042_init_model(struct max17042_chip *chip) static int max17042_verify_model_lock(struct max17042_chip *chip) { int i; - int table_size = - sizeof(chip->pdata->config_data->cell_char_tbl); + int table_size = ARRAY_SIZE(chip->pdata->config_data->cell_char_tbl); u16 *temp_data; int ret = 0; - temp_data = kzalloc(table_size, GFP_KERNEL); + temp_data = kcalloc(table_size, sizeof(*temp_data), GFP_KERNEL); if (!temp_data) return -ENOMEM; ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [patch] max17042_battery: fix a couple buffer overflows
On 03/15/2012 04:37 AM, Dan Carpenter wrote: There are a couple issues here caused by confusion between sizeof() and ARRAY_SIZE(). "table_size" should be the number of elements, but we should allocate it with kcalloc() so that we allocate the correct number of bytes. In max17042_init_model() we don't allocate enough space so we go past the end of the array in max17042_read_model_data() and max17042_model_data_compare(). In max17042_verify_model_lock() we allocate the right amount of space but we call max17042_read_model_data() with the wrong number of elements and also in the for loop we go past the end of the array. Signed-off-by: Dan Carpenter Acked-by: Dirk Brandewie --- I don't have this hardware. The original code is clearly buggy, but I can't test that my fix is correct. Please review carefully. diff --git a/drivers/power/max17042_battery.c b/drivers/power/max17042_battery.c index e36763a..f8cd48c 100644 --- a/drivers/power/max17042_battery.c +++ b/drivers/power/max17042_battery.c @@ -328,11 +328,10 @@ static inline int max17042_model_data_compare(struct max17042_chip *chip, static int max17042_init_model(struct max17042_chip *chip) { int ret; - int table_size = - sizeof(chip->pdata->config_data->cell_char_tbl)/sizeof(u16); + int table_size = ARRAY_SIZE(chip->pdata->config_data->cell_char_tbl); u16 *temp_data; - temp_data = kzalloc(table_size, GFP_KERNEL); + temp_data = kcalloc(table_size, sizeof(*temp_data), GFP_KERNEL); if (!temp_data) return -ENOMEM; @@ -357,12 +356,11 @@ static int max17042_init_model(struct max17042_chip *chip) static int max17042_verify_model_lock(struct max17042_chip *chip) { int i; - int table_size = - sizeof(chip->pdata->config_data->cell_char_tbl); + int table_size = ARRAY_SIZE(chip->pdata->config_data->cell_char_tbl); u16 *temp_data; int ret = 0; - temp_data = kzalloc(table_size, GFP_KERNEL); + temp_data = kcalloc(table_size, sizeof(*temp_data), GFP_KERNEL); if (!temp_data) return -ENOMEM; ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss