| Message ID | 20230525194255.4516-1-forst@pen.gy (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [net-next,v2,1/2] usbnet: ipheth: fix risk of NULL pointer deallocation | expand |
On Thu, May 25, 2023 at 09:42:54PM +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. Rearrange the goto labels to > avoid this risk. Hi Georgi and Foster, kfree will ignore a NULL argument, so I think the existing code is safe. But given the name of the label I do agree there is scope for a cleanup here. Could you consider rewording the patch description accordingly? > Signed-off-by: Georgi Valkov <gvalkov@gmail.com> If Georgi is the author of the patch, which seems to be the case, then the above is correct. But as the patch is being posted by Foster I think it should be followed by a Signed-off-by line for Foster. Link: https://www.kernel.org/doc/html/latest/process/submitting-patches.html?highlight=signed+off#developer-s-certificate-of-origin-1-1 > --- > drivers/net/usb/ipheth.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > 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;
> On 26 May 2023, at 10:52 AM, Simon Horman <simon.horman@corigine.com> wrote: > > On Thu, May 25, 2023 at 09:42:54PM +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. Rearrange the goto labels to >> avoid this risk. > > Hi Georgi and Foster, > > kfree will ignore a NULL argument, so I think the existing code is safe. > But given the name of the label I do agree there is scope for a cleanup > here. It’s good to know that precaution has been taken in kfree to avoid this, yet at my opinion knowingly attempting to free a NULL pointer is a red flag and bad design. Likely a misplaced label. > Could you consider rewording the patch description accordingly? What would you like me to use as title and description? Can I use this? usbnet: ipheth: avoid kfree with a NULL pointer 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> > > If Georgi is the author of the patch, which seems to be the case, > then the above is correct. But as the patch is being posted by Foster > I think it should be followed by a Signed-off-by line for Foster. Yes, I discovered the potential issue and authored the patch to help. We’ll append Signed-off-by Foster as you suggested. Thanks Simon! Something like that? Georgi Valkov httpstorm.com nano RTOS > Link: https://www.kernel.org/doc/html/latest/process/submitting-patches.html?highlight=signed+off#developer-s-certificate-of-origin-1-1 > >> --- >> drivers/net/usb/ipheth.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> 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; > > -- > pw-bot: cr
On Fri, May 26, 2023 at 10:33:21AM +0200, George Valkov wrote: > > > On 26 May 2023, at 10:52 AM, Simon Horman <simon.horman@corigine.com> wrote: > > > > On Thu, May 25, 2023 at 09:42:54PM +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. Rearrange the goto labels to > >> avoid this risk. > > > > Hi Georgi and Foster, > > > > kfree will ignore a NULL argument, so I think the existing code is safe. > > But given the name of the label I do agree there is scope for a cleanup > > here. > > It’s good to know that precaution has been taken in kfree to avoid this, yet at > my opinion knowingly attempting to free a NULL pointer is a red flag and bad > design. Likely a misplaced label. > > > Could you consider rewording the patch description accordingly? > > What would you like me to use as title and description? Can I use this? > > usbnet: ipheth: avoid kfree with a NULL pointer > > 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. Thanks, that looks good to me. > >> Signed-off-by: Georgi Valkov <gvalkov@gmail.com> > > > > If Georgi is the author of the patch, which seems to be the case, > > then the above is correct. But as the patch is being posted by Foster > > I think it should be followed by a Signed-off-by line for Foster. > > Yes, I discovered the potential issue and authored the patch to help. We’ll > append Signed-off-by Foster as you suggested. Thanks Simon! > > Something like that? Yes, I think that sounds good. Please wait 24h before the posting of v2 before posting v3, to allow time for more review of v3 (from others). ...
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;