Hi Heiko,
On 6/25/24 3:31 오후, Heiko Carstens wrote:
> On Tue, Jun 25, 2024 at 10:32:26AM +0900, yskelg@xxxxxxxxx wrote:
>> From: Yunseong Kim <yskelg@xxxxxxxxx>
>>
>> A null pointer is stored in a local variable after a call of the function
>> "kzalloc" failed. This pointer was passed to a subsequent call of the
>> function "raw3270_setup_device" where an undesirable dereference will be
>> performed then. Thus add corresponding return value checks.
>> The allocated each memory areas are immediately overwritten by the called
>> function zero-initialisation be omitted by calling the "kmalloc" instead.
>> After "ccw_device_enable_console" succeeds, set the bit raw3270 flag to
>> RAW3270_FLAGS_CONSOLE.
>>
>> Fixes: 33403dcfcdfd ("[S390] 3270 console: convert from bootmem to slab")
>> Cc: linux-s390@xxxxxxxxxxxxxxx
>> Signed-off-by: Yunseong Kim <yskelg@xxxxxxxxx>
>> ---
>> drivers/s390/char/raw3270.c | 20 +++++++++++++++-----
>> 1 file changed, 15 insertions(+), 5 deletions(-)
> ...
>> rc = raw3270_setup_device(cdev, rp, ascebc);
>> - if (rc)
>> + if (rc) {
>> + kfree(ascebc);
>> + kfree(rp);
>> return ERR_PTR(rc);
>> - set_bit(RAW3270_FLAGS_CONSOLE, &rp->flags);
>> -
>> + }
>> rc = ccw_device_enable_console(cdev);
>> if (rc) {
>> ccw_device_destroy_console(cdev);
>> + kfree(ascebc);
>> + kfree(rp);
>> return ERR_PTR(rc);
>> }
>> + set_bit(RAW3270_FLAGS_CONSOLE, &rp->flags);
>
> Why did you move the set_bit() call?
Thank you for the code review Heiko.
While writing patch version 2, I spent a lot of time thinking about this
part. Previously, even if function "ccw_device_enable_console" failed,
the flag was set to RAW3270_FLAGS_CONSOLE and returned.
I think it would be more appropriate to set the bit after everything
succeeded, so I included and submitted this code in v2 patch.
I’d appreciate hearing your thoughts on this!
Warm regards,
Yunseong Kim
