[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Fri, 9 Mar 2018 14:04:44 +0100
From: Krzysztof Kozlowski <krzk@...nel.org>
To: "Gustavo A. R. Silva" <garsilva@...eddedor.com>
Cc: Kees Cook <keescook@...omium.org>, "Gustavo A. R. Silva" <gustavo@...eddedor.com>,
Sangbeom Kim <sbkim73@...sung.com>, Bartlomiej Zolnierkiewicz <b.zolnierkie@...sung.com>,
Alessandro Zummo <a.zummo@...ertech.it>, Alexandre Belloni <alexandre.belloni@...tlin.com>,
LKML <linux-kernel@...r.kernel.org>, linux-samsung-soc@...r.kernel.org,
linux-rtc@...r.kernel.org,
Kernel Hardening <kernel-hardening@...ts.openwall.com>
Subject: Re: [PATCH] rtc: s5m: Remove VLA usage
On Thu, Mar 8, 2018 at 7:03 PM, Gustavo A. R. Silva
<garsilva@...eddedor.com> wrote:
>
>
> On 03/08/2018 11:58 AM, Kees Cook wrote:
>>
>> On Thu, Mar 8, 2018 at 9:20 AM, Gustavo A. R. Silva
>> <gustavo@...eddedor.com> wrote:
>>>
>>> In preparation to enabling -Wvla, remove VLAs and replace them
>>> with fixed-length arrays instead.
>>>
>>> From a security viewpoint, the use of Variable Length Arrays can be
>>> a vector for stack overflow attacks. Also, in general, as the code
>>> evolves it is easy to lose track of how big a VLA can get. Thus, we
>>> can end up having segfaults that are hard to debug.
>>>
>>> Also, fixed as part of the directive to remove all VLAs from
>>> the kernel: https://lkml.org/lkml/2018/3/7/621
>>>
>>> Signed-off-by: Gustavo A. R. Silva <gustavo@...eddedor.com>
>>> ---
>>> drivers/rtc/rtc-s5m.c | 15 +++++++++------
>>> 1 file changed, 9 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/rtc/rtc-s5m.c b/drivers/rtc/rtc-s5m.c
>>> index 6deae10..2b5f4f7 100644
>>> --- a/drivers/rtc/rtc-s5m.c
>>> +++ b/drivers/rtc/rtc-s5m.c
>>> @@ -38,6 +38,9 @@
>>> */
>>> #define UDR_READ_RETRY_CNT 5
>>>
>>> +/* Maximum number of registers for setting time/alarm0/alarm1 */
>>> +#define MAX_NUM_TIME_REGS 8
>>
>>
>> I would adjust the various const struct s5m_rtc_reg_config's
>> .regs_count to be represented by this new define, so the stack and the
>> structures stay in sync. Something like:
>>
>> static const struct s5m_rtc_reg_config s2mps13_rtc_regs = {
>> .regs_count = MAX_NUM_TIME_REGS - 1,
>>
>> ?
>>
>
> Yep. I thought about that and decided to wait for some feedback first. But
> yeah, I think is that'd be a good change.
Define and these assignments should be somehow connected with enum
defining the offsets for data[] (from
include/linux/mfd/samsung/rtc.h). Otherwise we define the same in two
places. The enum could be itself (in separate patch) moved to the
driver because it is meaningless for others.
Best regards,
Krzysztof
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
