| Message ID | 20230527130309.34090-1-forst@pen.gy (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [net-next,v3,1/2] usbnet: ipheth: fix risk of NULL pointer deallocation | expand |
On Sat, May 27, 2023 at 03:03:08PM +0200, Foster Snowhill wrote: > From: Georgi Valkov <gvalkov@gmail.com> > > The cleanup precedure in ipheth_probe will attempt to free a > NULL pointer in dev->ctrl_buf if the memory allocation for > this buffer is not successful. While kfree ignores NULL pointers, > and the existing code is safe, it is a better design to rearrange > the goto labels and avoid this. > > Signed-off-by: Georgi Valkov <gvalkov@gmail.com> > Signed-off-by: Foster Snowhill <forst@pen.gy> Reviewed-by: Simon Horman <simon.horman@corigine.com>
Hi, On Sat, 2023-05-27 at 15:03 +0200, Foster Snowhill wrote: > From: Georgi Valkov <gvalkov@gmail.com> > > The cleanup precedure in ipheth_probe will attempt to free a > NULL pointer in dev->ctrl_buf if the memory allocation for > this buffer is not successful. While kfree ignores NULL pointers, > and the existing code is safe, it is a better design to rearrange > the goto labels and avoid this. > > Signed-off-by: Georgi Valkov <gvalkov@gmail.com> > Signed-off-by: Foster Snowhill <forst@pen.gy> If you are going to repost (due to changes in patch 2) please update this patch subj, too. Currently is a bit confusing, something alike "cleanup the initialization error path" would be more clear. Thanks, Paolo
Thanks Paolo! Something like that? Georgi Valkov httpstorm.com nano RTOS > On 30 May 2023, at 2:02 PM, Paolo Abeni <pabeni@redhat.com> wrote: > > Hi, > > On Sat, 2023-05-27 at 15:03 +0200, Foster Snowhill wrote: >> From: Georgi Valkov <gvalkov@gmail.com> >> >> The cleanup precedure in ipheth_probe will attempt to free a >> NULL pointer in dev->ctrl_buf if the memory allocation for >> this buffer is not successful. While kfree ignores NULL pointers, >> and the existing code is safe, it is a better design to rearrange >> the goto labels and avoid this. >> >> Signed-off-by: Georgi Valkov <gvalkov@gmail.com> >> Signed-off-by: Foster Snowhill <forst@pen.gy> > > If you are going to repost (due to changes in patch 2) please update > this patch subj, too. Currently is a bit confusing, something alike > "cleanup the initialization error path" would be more clear. > > Thanks, > > Paolo >
Hi Paolo! Sorry, I attached the old version by mistake. Here is the new version: Georgi Valkov httpstorm.com nano RTOS > On 30 May 2023, at 2:02 PM, Paolo Abeni <pabeni@redhat.com> wrote: > > Hi, > > On Sat, 2023-05-27 at 15:03 +0200, Foster Snowhill wrote: >> From: Georgi Valkov <gvalkov@gmail.com> >> >> The cleanup precedure in ipheth_probe will attempt to free a >> NULL pointer in dev->ctrl_buf if the memory allocation for >> this buffer is not successful. While kfree ignores NULL pointers, >> and the existing code is safe, it is a better design to rearrange >> the goto labels and avoid this. >> >> Signed-off-by: Georgi Valkov <gvalkov@gmail.com> >> Signed-off-by: Foster Snowhill <forst@pen.gy> > > If you are going to repost (due to changes in patch 2) please update > this patch subj, too. Currently is a bit confusing, something alike > "cleanup the initialization error path" would be more clear. > > Thanks, > > Paolo >
On Tue, 2023-05-30 at 14:11 +0300, George Valkov wrote:
> Sorry, I attached the old version by mistake. Here is the new version:
LGTM.
Please in future prefer inline patch vs attachments even for
discussion.
Note that you will have to formally repost both patches.
Thanks!
Paolo
diff --git a/drivers/net/usb/ipheth.c b/drivers/net/usb/ipheth.c index 6a769df0b..8875a3d0e 100644 --- a/drivers/net/usb/ipheth.c +++ b/drivers/net/usb/ipheth.c @@ -510,8 +510,8 @@ static int ipheth_probe(struct usb_interface *intf, ipheth_free_urbs(dev); err_alloc_urbs: err_get_macaddr: -err_alloc_ctrl_buf: kfree(dev->ctrl_buf); +err_alloc_ctrl_buf: err_endpoints: free_netdev(netdev); return retval;