| Message ID | CAO9wTFgtDGMxgE0QFu7CjhsMzqOm0ydV548j4ZjYz+SCgcRY3Q@mail.gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: dev_addr_list: add address length validation in __hw_addr_insert function | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Guessing tree name failed - patch did not apply |
On Mon, Feb 17, 2025 at 5:54 PM Suchit K <suchitkarunakaran@gmail.com> wrote: > > Add validation checks for hardware address length in > __hw_addr_insert() to prevent problems with invalid lengths. > > Signed-off-by: Suchit Karunakaran <suchitkarunakaran@gmail.com> > --- > net/core/dev_addr_lists.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/net/core/dev_addr_lists.c b/net/core/dev_addr_lists.c > index 90716bd73..b6b906b2a 100644 > --- a/net/core/dev_addr_lists.c > +++ b/net/core/dev_addr_lists.c > @@ -21,6 +21,9 @@ > static int __hw_addr_insert(struct netdev_hw_addr_list *list, > struct netdev_hw_addr *new, int addr_len) > { > + if (!list || !new || addr_len <= 0 || addr_len > MAX_ADDR_LEN) > + return -EINVAL; > + We do not put code before variable declarations. Also, why @list would be NULL, or @new being NULL ? This does not match the changelog. > struct rb_node **ins_point = &list->tree.rb_node, *parent = NULL; > struct netdev_hw_addr *ha; > Any syzbot report to share with us ? Also, a Fixes: tag would be needed.
Hi Eric, Thanks for the feedback! I'm new to kernel development and still finding my way around. I wasn't working from a syzbot report on this one; I was just exploring the code and felt there is no parameter validation. I went ahead and made this change based on that impression. I realized my changelog should have been more generic. Sorry about that. Also since it's not based on a syzbot report, is it good to have this change? Your insights and suggestions would be most welcome. I will make the required changes accordingly. Thanks. On Mon, 17 Feb 2025 at 23:58, Eric Dumazet <edumazet@google.com> wrote: > > On Mon, Feb 17, 2025 at 5:54 PM Suchit K <suchitkarunakaran@gmail.com> wrote: > > > > Add validation checks for hardware address length in > > __hw_addr_insert() to prevent problems with invalid lengths. > > > > Signed-off-by: Suchit Karunakaran <suchitkarunakaran@gmail.com> > > --- > > net/core/dev_addr_lists.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/net/core/dev_addr_lists.c b/net/core/dev_addr_lists.c > > index 90716bd73..b6b906b2a 100644 > > --- a/net/core/dev_addr_lists.c > > +++ b/net/core/dev_addr_lists.c > > @@ -21,6 +21,9 @@ > > static int __hw_addr_insert(struct netdev_hw_addr_list *list, > > struct netdev_hw_addr *new, int addr_len) > > { > > + if (!list || !new || addr_len <= 0 || addr_len > MAX_ADDR_LEN) > > + return -EINVAL; > > + > > We do not put code before variable declarations. > > Also, why @list would be NULL, or @new being NULL ? > This does not match the changelog. > > > struct rb_node **ins_point = &list->tree.rb_node, *parent = NULL; > > struct netdev_hw_addr *ha; > > > > Any syzbot report to share with us ? > > Also, a Fixes: tag would be needed.
On Mon, Feb 17, 2025 at 8:05 PM Suchit K <suchitkarunakaran@gmail.com> wrote: > > Hi Eric, > Thanks for the feedback! I'm new to kernel development and still > finding my way around. > I wasn't working from a syzbot report on this one; I was just > exploring the code and felt there is no parameter validation. I went > ahead and made this change based on that impression. I realized my > changelog should have been more generic. Sorry about that. Also since > it's not based on a syzbot report, is it good to have this change? > Your insights and suggestions would be most welcome. I will make the > required changes accordingly. > Thanks. I think these checks are not necessary. 1) The caller (dev_addr_mod) provides non NULL pointers, there is no point adding tests, because if one of them was NULL, a crash would occur before hitting this function. 2) Your patch would silently hide a real issue if for some reason dev->addr_len was too big.
Thank you so much for the feedback. I appreciate your time and effort in reviewing and providing feedback. On Tue, 18 Feb 2025 at 00:51, Eric Dumazet <edumazet@google.com> wrote: > > On Mon, Feb 17, 2025 at 8:05 PM Suchit K <suchitkarunakaran@gmail.com> wrote: > > > > Hi Eric, > > Thanks for the feedback! I'm new to kernel development and still > > finding my way around. > > I wasn't working from a syzbot report on this one; I was just > > exploring the code and felt there is no parameter validation. I went > > ahead and made this change based on that impression. I realized my > > changelog should have been more generic. Sorry about that. Also since > > it's not based on a syzbot report, is it good to have this change? > > Your insights and suggestions would be most welcome. I will make the > > required changes accordingly. > > Thanks. > > I think these checks are not necessary. > > 1) The caller (dev_addr_mod) provides non NULL pointers, > there is no point adding tests, because if one of them was NULL, > a crash would occur before hitting this function. > > 2) Your patch would silently hide a real issue if for some reason > dev->addr_len was too big.
diff --git a/net/core/dev_addr_lists.c b/net/core/dev_addr_lists.c index 90716bd73..b6b906b2a 100644 --- a/net/core/dev_addr_lists.c +++ b/net/core/dev_addr_lists.c @@ -21,6 +21,9 @@ static int __hw_addr_insert(struct netdev_hw_addr_list *list, struct netdev_hw_addr *new, int addr_len) { + if (!list || !new || addr_len <= 0 || addr_len > MAX_ADDR_LEN) + return -EINVAL; + struct rb_node **ins_point = &list->tree.rb_node, *parent = NULL; struct netdev_hw_addr *ha;
Add validation checks for hardware address length in __hw_addr_insert() to prevent problems with invalid lengths. Signed-off-by: Suchit Karunakaran <suchitkarunakaran@gmail.com> --- net/core/dev_addr_lists.c | 3 +++ 1 file changed, 3 insertions(+)