On Wed, Feb 14, 2018 at 04:49:51PM -0700, Jason Gunthorpe wrote:
> On Wed, Feb 14, 2018 at 02:38:37PM +0200, Leon Romanovsky wrote:
> > From: Leon Romanovsky <leonro@xxxxxxxxxxxx>
> >
> > Move all command header processing into separate function
> > and perform those checks before acquiring SRCU read lock.
> >
> > Signed-off-by: Leon Romanovsky <leonro@xxxxxxxxxxxx>
> > drivers/infiniband/core/uverbs_main.c | 62 ++++++++++++++++++-----------------
> > 1 file changed, 32 insertions(+), 30 deletions(-)
> >
> > diff --git a/drivers/infiniband/core/uverbs_main.c b/drivers/infiniband/core/uverbs_main.c
> > index 90e0b16aed1a..50d43c300112 100644
> > +++ b/drivers/infiniband/core/uverbs_main.c
> > @@ -660,6 +660,29 @@ static bool verify_command_idx(__u32 command, bool extended)
> > uverbs_cmd_table[command];
> > }
> >
> > +static ssize_t process_hdr(struct ib_uverbs_cmd_hdr *hdr,
> > + __u32 *command, bool *extended)
> > +{
> > + __u32 flags;
> > +
> > + if (hdr->command & ~(__u32)(IB_USER_VERBS_CMD_FLAGS_MASK |
> > + IB_USER_VERBS_CMD_COMMAND_MASK))
> > + return -EINVAL;
> > +
> > + *command = hdr->command & IB_USER_VERBS_CMD_COMMAND_MASK;
> > + flags = (hdr->command &
> > + IB_USER_VERBS_CMD_FLAGS_MASK) >> IB_USER_VERBS_CMD_FLAGS_SHIFT;
> > +
> > + *extended = flags & IB_USER_VERBS_CMD_FLAG_EXTENDED;
> > + if (flags & ~IB_USER_VERBS_CMD_FLAG_EXTENDED)
> > + return -EINVAL;
>
> er..
>
> We don't need both
>
> if (hdr->command & ~(__u32)(IB_USER_VERBS_CMD_FLAGS_MASK |
> IB_USER_VERBS_CMD_COMMAND_MASK))
>
> and
>
> if (flags & ~IB_USER_VERBS_CMD_FLAG_EXTENDED)
> return -EINVAL;
>
> They test the same condition.
Not exactly,
~(__u32)(IB_USER_VERBS_CMD_FLAGS_MASK | IB_USER_VERBS_CMD_COMMAND_MASK)
== ~(__u32)(0xff000000u| 0xff) == 0x00ffff00
=> we are testing that middle bits of hdr->command are cleared
flags & ~IB_USER_VERBS_CMD_FLAG_EXTENDED == ((hdr->command &
IB_USER_VERBS_CMD_FLAGS_MASK) >> IB_USER_VERBS_CMD_FLAGS_SHIFT) &
~IB_USER_VERBS_CMD_FLAG_EXTENDED == 0xff000000u >> 24 & ~0x80 ==
0xff & 0x7F == 0x7F
=> we are testing that only extended flag is enable.
>
> So my earlier comment about needing a stronger flags check is bogus -
> we already have the stronger check. The original code was just
> checking it twice, so it doesn't need this extra if.
>
> Jason
Attachment:
signature.asc
Description: PGP signature
