Re: [PATCH] ipmi: fix unsigned long underflow

[Corey Minyard]

On 08/07/2017 03:41 AM, Weilong Chen wrote:
> Hi Minyard,
>
> I test this patch, it works.
>
> Thanks.

Thanks, I added a Tested-by: for you and it's queued for the next release.
If it's urgent I can send it in now.

I also added this for the stable kernels 3.16 and later.

-corey

> On 2017/7/30 10:20, minyard@xxxxxxx wrote:
> > From: Corey Minyard <cminyard@xxxxxxxxxx>
> >
> > When I set the timeout to a specific value such as 500ms, the timeout
> > event will not happen in time due to the overflow in function
> > check_msg_timeout:
> > ...
> >     ent->timeout -= timeout_period;
> >     if (ent->timeout > 0)
> >         return;
> > ...
> >
> > The type of timeout_period is long, but ent->timeout is unsigned long.
> > This patch makes the type consistent.
> >
> > Reported-by: Weilong Chen <chenweilong@xxxxxxxxxx>
> > Signed-off-by: Corey Minyard <cminyard@xxxxxxxxxx>
> > ---
> > I like to keep things consistent (though I obviously messed up here)
> > and keep variables that should be positive unsigned.
> >
> > But you are right, there is a bug here and some inconsistency.
> > This patch changes timeout_period to be unsigned and fixes the
> > check.  Can you try this out?
> >
> >  drivers/char/ipmi/ipmi_msghandler.c | 10 ++++++----
> >  1 file changed, 6 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/char/ipmi/ipmi_msghandler.c 
> > b/drivers/char/ipmi/ipmi_msghandler.c
> > index 810b138..c82d9fd 100644
> > --- a/drivers/char/ipmi/ipmi_msghandler.c
> > +++ b/drivers/char/ipmi/ipmi_msghandler.c
> > @@ -4030,7 +4030,8 @@ smi_from_recv_msg(ipmi_smi_t intf, struct 
> > ipmi_recv_msg *recv_msg,
> >  }
> >
> >  static void check_msg_timeout(ipmi_smi_t intf, struct seq_table *ent,
> > -                  struct list_head *timeouts, long timeout_period,
> > +                  struct list_head *timeouts,
> > +                  unsigned long timeout_period,
> >                    int slot, unsigned long *flags,
> >                    unsigned int *waiting_msgs)
> >  {
> > @@ -4043,8 +4044,8 @@ static void check_msg_timeout(ipmi_smi_t intf, 
> > struct seq_table *ent,
> >      if (!ent->inuse)
> >          return;
> >
> > -    ent->timeout -= timeout_period;
> > -    if (ent->timeout > 0) {
> > +    if (timeout_period < ent->timeout) {
> > +        ent->timeout -= timeout_period;
> >          (*waiting_msgs)++;
> >          return;
> >      }
> > @@ -4110,7 +4111,8 @@ static void check_msg_timeout(ipmi_smi_t intf, 
> > struct seq_table *ent,
> >      }
> >  }
> >
> > -static unsigned int ipmi_timeout_handler(ipmi_smi_t intf, long 
> > timeout_period)
> > +static unsigned int ipmi_timeout_handler(ipmi_smi_t intf,
> > +                     unsigned long timeout_period)
> >  {
> >      struct list_head     timeouts;
> >      struct ipmi_recv_msg *msg, *msg2;