Re: [PATCH][next] platform/chrome: wilco_ec: fix null pointer dereference on failed kzalloc
On Tue, Jun 18, 2019 at 11:30 PM Dan Carpenter wrote: > > On Tue, Jun 18, 2019 at 04:39:24PM +0100, Colin King wrote: > > diff --git a/drivers/platform/chrome/wilco_ec/event.c > > b/drivers/platform/chrome/wilco_ec/event.c > > index c975b76e6255..e251a989b152 100644 > > --- a/drivers/platform/chrome/wilco_ec/event.c > > +++ b/drivers/platform/chrome/wilco_ec/event.c > > @@ -112,8 +112,11 @@ module_param(queue_size, int, 0644); > > static struct ec_event_queue *event_queue_new(int capacity) > > { > > size_t entries_size = sizeof(struct ec_event *) * capacity; > > - struct ec_event_queue *q = kzalloc(sizeof(*q) + entries_size, > > -GFP_KERNEL); > > + struct ec_event_queue *q; > > + > > + q = kzalloc(sizeof(*q) + entries_size, GFP_KERNEL); > > + if (!q) > > + return NULL; > > We have a new struct_size() macro designed for these allocations. > > q = kzalloc(struct_size(q, entries, capacity), GFP_KERNEL); > > The advantage is that it checks for integer overflows. > > regards, > dan carpenter > Thanks Dan, I like that. Dmitry Torokhov also had some thoughts on this patch at https://crrev.com/c/1661053, I'll send a patch that adds this and fixes his concerns in a bit. Cheers, Nick
Re: [PATCH][next] platform/chrome: wilco_ec: fix null pointer dereference on failed kzalloc
On Tue, Jun 18, 2019 at 04:39:24PM +0100, Colin King wrote: > diff --git a/drivers/platform/chrome/wilco_ec/event.c > b/drivers/platform/chrome/wilco_ec/event.c > index c975b76e6255..e251a989b152 100644 > --- a/drivers/platform/chrome/wilco_ec/event.c > +++ b/drivers/platform/chrome/wilco_ec/event.c > @@ -112,8 +112,11 @@ module_param(queue_size, int, 0644); > static struct ec_event_queue *event_queue_new(int capacity) > { > size_t entries_size = sizeof(struct ec_event *) * capacity; > - struct ec_event_queue *q = kzalloc(sizeof(*q) + entries_size, > -GFP_KERNEL); > + struct ec_event_queue *q; > + > + q = kzalloc(sizeof(*q) + entries_size, GFP_KERNEL); > + if (!q) > + return NULL; We have a new struct_size() macro designed for these allocations. q = kzalloc(struct_size(q, entries, capacity), GFP_KERNEL); The advantage is that it checks for integer overflows. regards, dan carpenter
Re: [PATCH][next] platform/chrome: wilco_ec: fix null pointer dereference on failed kzalloc
Hi Colin, On Tue, Jun 18, 2019 at 04:39:24PM +0100, Colin King wrote: > From: Colin Ian King > > If the kzalloc of the entries queue q fails a null pointer dereference > occurs when accessing q->capacity and q->lock. Add a kzalloc failure > check and handle the null return case in the calling function > event_device_add. > > Addresses-Coverity: ("Dereference null return") > Fixes: 75589e37d1dc ("platform/chrome: wilco_ec: Add circular buffer as event > queue") > Signed-off-by: Colin Ian King Applied. Thanks. Benson > --- > drivers/platform/chrome/wilco_ec/event.c | 12 ++-- > 1 file changed, 10 insertions(+), 2 deletions(-) > > diff --git a/drivers/platform/chrome/wilco_ec/event.c > b/drivers/platform/chrome/wilco_ec/event.c > index c975b76e6255..e251a989b152 100644 > --- a/drivers/platform/chrome/wilco_ec/event.c > +++ b/drivers/platform/chrome/wilco_ec/event.c > @@ -112,8 +112,11 @@ module_param(queue_size, int, 0644); > static struct ec_event_queue *event_queue_new(int capacity) > { > size_t entries_size = sizeof(struct ec_event *) * capacity; > - struct ec_event_queue *q = kzalloc(sizeof(*q) + entries_size, > -GFP_KERNEL); > + struct ec_event_queue *q; > + > + q = kzalloc(sizeof(*q) + entries_size, GFP_KERNEL); > + if (!q) > + return NULL; > > q->capacity = capacity; > spin_lock_init(>lock); > @@ -474,6 +477,11 @@ static int event_device_add(struct acpi_device *adev) > /* Initialize the device data. */ > adev->driver_data = dev_data; > dev_data->events = event_queue_new(queue_size); > + if (!dev_data->events) { > + kfree(dev_data); > + error = -ENOMEM; > + goto free_minor; > + } > init_waitqueue_head(_data->wq); > dev_data->exist = true; > atomic_set(_data->available, 1); > -- > 2.20.1 > -- Benson Leung Staff Software Engineer Chrome OS Kernel Google Inc. ble...@google.com Chromium OS Project ble...@chromium.org signature.asc Description: PGP signature
Re: [PATCH][next] platform/chrome: wilco_ec: fix null pointer dereference on failed kzalloc
Hi Nick, On Tue, Jun 18, 2019 at 11:15:03AM -0600, Nick Crews wrote: > Thanks Colin, good catch. > > Enric, could you squash this into the real commit? I've applied this to for-next and for-kernelci in chrome-platform. Thanks, Benson -- Benson Leung Staff Software Engineer Chrome OS Kernel Google Inc. ble...@google.com Chromium OS Project ble...@chromium.org signature.asc Description: PGP signature
Re: [PATCH][next] platform/chrome: wilco_ec: fix null pointer dereference on failed kzalloc
Thanks Colin, good catch. Enric, could you squash this into the real commit? On Tue, Jun 18, 2019 at 9:39 AM Colin King wrote: > > From: Colin Ian King > > If the kzalloc of the entries queue q fails a null pointer dereference > occurs when accessing q->capacity and q->lock. Add a kzalloc failure > check and handle the null return case in the calling function > event_device_add. > > Addresses-Coverity: ("Dereference null return") > Fixes: 75589e37d1dc ("platform/chrome: wilco_ec: Add circular buffer as event > queue") > Signed-off-by: Colin Ian King > --- > drivers/platform/chrome/wilco_ec/event.c | 12 ++-- > 1 file changed, 10 insertions(+), 2 deletions(-) > > diff --git a/drivers/platform/chrome/wilco_ec/event.c > b/drivers/platform/chrome/wilco_ec/event.c > index c975b76e6255..e251a989b152 100644 > --- a/drivers/platform/chrome/wilco_ec/event.c > +++ b/drivers/platform/chrome/wilco_ec/event.c > @@ -112,8 +112,11 @@ module_param(queue_size, int, 0644); > static struct ec_event_queue *event_queue_new(int capacity) > { > size_t entries_size = sizeof(struct ec_event *) * capacity; > - struct ec_event_queue *q = kzalloc(sizeof(*q) + entries_size, > - GFP_KERNEL); > + struct ec_event_queue *q; > + > + q = kzalloc(sizeof(*q) + entries_size, GFP_KERNEL); > + if (!q) > + return NULL; > > q->capacity = capacity; > spin_lock_init(>lock); > @@ -474,6 +477,11 @@ static int event_device_add(struct acpi_device *adev) > /* Initialize the device data. */ > adev->driver_data = dev_data; > dev_data->events = event_queue_new(queue_size); > + if (!dev_data->events) { > + kfree(dev_data); > + error = -ENOMEM; > + goto free_minor; > + } > init_waitqueue_head(_data->wq); > dev_data->exist = true; > atomic_set(_data->available, 1); Signed-off-by: Nick Crews > -- > 2.20.1 >