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;
+
  struct rb_node **ins_point = &list->tree.rb_node, *parent = NULL;
  struct netdev_hw_addr *ha;

-- 
2.48.1

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.