| Message ID | 20250304140246.205919-1-qasdev00@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | RDMA/mlx5: Prevent UB from shifting negative signed value | expand |
On 3/4/2025 4:02 PM, Qasim Ijaz wrote: > External email: Use caution opening links or attachments > > > In function create_ib_ah() the following line attempts > to left shift the return value of mlx5r_ib_rate() by 4 > and store it in the stat_rate_sl member of av: > > ah->av.stat_rate_sl = (mlx5r_ib_rate(dev, rdma_ah_get_static_rate(ah_attr)) << 4); > > However the code overlooks the fact that mlx5r_ib_rate() > may return -EINVAL if the rate passed to it is less than > IB_RATE_2_5_GBPS or greater than IB_RATE_800_GBPS. > > Because of this, the code may invoke undefined behaviour when > shifting a signed negative value when doing "-EINVAL << 4". > > To fix this check for errors before assigning stat_rate_sl and > propagate any error value to the callers. > > Signed-off-by: Qasim Ijaz <qasdev00@gmail.com> > Fixes: c534ffda781f ("RDMA/mlx5: Fix AH static rate parsing") > Cc: stable@vger.kernel.org Thanks for fixing this , looks good to me. Reviewed-by: Patrisious Haddad <phaddad@nvidia.com> > --- > drivers/infiniband/hw/mlx5/ah.c | 17 +++++++++++++---- > 1 file changed, 13 insertions(+), 4 deletions(-) > > diff --git a/drivers/infiniband/hw/mlx5/ah.c b/drivers/infiniband/hw/mlx5/ah.c > index 99036afb3aef..6bccd9ce4538 100644 > --- a/drivers/infiniband/hw/mlx5/ah.c > +++ b/drivers/infiniband/hw/mlx5/ah.c > @@ -50,11 +50,12 @@ static __be16 mlx5_ah_get_udp_sport(const struct mlx5_ib_dev *dev, > return sport; > } > > -static void create_ib_ah(struct mlx5_ib_dev *dev, struct mlx5_ib_ah *ah, > +static int create_ib_ah(struct mlx5_ib_dev *dev, struct mlx5_ib_ah *ah, > struct rdma_ah_init_attr *init_attr) > { > struct rdma_ah_attr *ah_attr = init_attr->ah_attr; > enum ib_gid_type gid_type; > + int rate_val; > > if (rdma_ah_get_ah_flags(ah_attr) & IB_AH_GRH) { > const struct ib_global_route *grh = rdma_ah_read_grh(ah_attr); > @@ -67,8 +68,10 @@ static void create_ib_ah(struct mlx5_ib_dev *dev, struct mlx5_ib_ah *ah, > ah->av.tclass = grh->traffic_class; > } > > - ah->av.stat_rate_sl = > - (mlx5r_ib_rate(dev, rdma_ah_get_static_rate(ah_attr)) << 4); > + rate_val = mlx5r_ib_rate(dev, rdma_ah_get_static_rate(ah_attr)); > + if (rate_val < 0) > + return rate_val; > + ah->av.stat_rate_sl = rate_val << 4; > > if (ah_attr->type == RDMA_AH_ATTR_TYPE_ROCE) { > if (init_attr->xmit_slave) > @@ -89,6 +92,8 @@ static void create_ib_ah(struct mlx5_ib_dev *dev, struct mlx5_ib_ah *ah, > ah->av.fl_mlid = rdma_ah_get_path_bits(ah_attr) & 0x7f; > ah->av.stat_rate_sl |= (rdma_ah_get_sl(ah_attr) & 0xf); > } > + > + return 0; > } > > int mlx5_ib_create_ah(struct ib_ah *ibah, struct rdma_ah_init_attr *init_attr, > @@ -99,6 +104,7 @@ int mlx5_ib_create_ah(struct ib_ah *ibah, struct rdma_ah_init_attr *init_attr, > struct mlx5_ib_ah *ah = to_mah(ibah); > struct mlx5_ib_dev *dev = to_mdev(ibah->device); > enum rdma_ah_attr_type ah_type = ah_attr->type; > + int ret; > > if ((ah_type == RDMA_AH_ATTR_TYPE_ROCE) && > !(rdma_ah_get_ah_flags(ah_attr) & IB_AH_GRH)) > @@ -121,7 +127,10 @@ int mlx5_ib_create_ah(struct ib_ah *ibah, struct rdma_ah_init_attr *init_attr, > return err; > } > > - create_ib_ah(dev, ah, init_attr); > + ret = create_ib_ah(dev, ah, init_attr); > + if (ret) > + return ret; > + > return 0; > } > > -- > 2.39.5 > >
On Tue, 04 Mar 2025 14:02:46 +0000, Qasim Ijaz wrote: > In function create_ib_ah() the following line attempts > to left shift the return value of mlx5r_ib_rate() by 4 > and store it in the stat_rate_sl member of av: > > ah->av.stat_rate_sl = (mlx5r_ib_rate(dev, rdma_ah_get_static_rate(ah_attr)) << 4); > > However the code overlooks the fact that mlx5r_ib_rate() > may return -EINVAL if the rate passed to it is less than > IB_RATE_2_5_GBPS or greater than IB_RATE_800_GBPS. > > [...] Applied, thanks! [1/1] RDMA/mlx5: Prevent UB from shifting negative signed value https://git.kernel.org/rdma/rdma/c/556f93b90c1872 Best regards,
diff --git a/drivers/infiniband/hw/mlx5/ah.c b/drivers/infiniband/hw/mlx5/ah.c index 99036afb3aef..6bccd9ce4538 100644 --- a/drivers/infiniband/hw/mlx5/ah.c +++ b/drivers/infiniband/hw/mlx5/ah.c @@ -50,11 +50,12 @@ static __be16 mlx5_ah_get_udp_sport(const struct mlx5_ib_dev *dev, return sport; } -static void create_ib_ah(struct mlx5_ib_dev *dev, struct mlx5_ib_ah *ah, +static int create_ib_ah(struct mlx5_ib_dev *dev, struct mlx5_ib_ah *ah, struct rdma_ah_init_attr *init_attr) { struct rdma_ah_attr *ah_attr = init_attr->ah_attr; enum ib_gid_type gid_type; + int rate_val; if (rdma_ah_get_ah_flags(ah_attr) & IB_AH_GRH) { const struct ib_global_route *grh = rdma_ah_read_grh(ah_attr); @@ -67,8 +68,10 @@ static void create_ib_ah(struct mlx5_ib_dev *dev, struct mlx5_ib_ah *ah, ah->av.tclass = grh->traffic_class; } - ah->av.stat_rate_sl = - (mlx5r_ib_rate(dev, rdma_ah_get_static_rate(ah_attr)) << 4); + rate_val = mlx5r_ib_rate(dev, rdma_ah_get_static_rate(ah_attr)); + if (rate_val < 0) + return rate_val; + ah->av.stat_rate_sl = rate_val << 4; if (ah_attr->type == RDMA_AH_ATTR_TYPE_ROCE) { if (init_attr->xmit_slave) @@ -89,6 +92,8 @@ static void create_ib_ah(struct mlx5_ib_dev *dev, struct mlx5_ib_ah *ah, ah->av.fl_mlid = rdma_ah_get_path_bits(ah_attr) & 0x7f; ah->av.stat_rate_sl |= (rdma_ah_get_sl(ah_attr) & 0xf); } + + return 0; } int mlx5_ib_create_ah(struct ib_ah *ibah, struct rdma_ah_init_attr *init_attr, @@ -99,6 +104,7 @@ int mlx5_ib_create_ah(struct ib_ah *ibah, struct rdma_ah_init_attr *init_attr, struct mlx5_ib_ah *ah = to_mah(ibah); struct mlx5_ib_dev *dev = to_mdev(ibah->device); enum rdma_ah_attr_type ah_type = ah_attr->type; + int ret; if ((ah_type == RDMA_AH_ATTR_TYPE_ROCE) && !(rdma_ah_get_ah_flags(ah_attr) & IB_AH_GRH)) @@ -121,7 +127,10 @@ int mlx5_ib_create_ah(struct ib_ah *ibah, struct rdma_ah_init_attr *init_attr, return err; } - create_ib_ah(dev, ah, init_attr); + ret = create_ib_ah(dev, ah, init_attr); + if (ret) + return ret; + return 0; }
In function create_ib_ah() the following line attempts to left shift the return value of mlx5r_ib_rate() by 4 and store it in the stat_rate_sl member of av: ah->av.stat_rate_sl = (mlx5r_ib_rate(dev, rdma_ah_get_static_rate(ah_attr)) << 4); However the code overlooks the fact that mlx5r_ib_rate() may return -EINVAL if the rate passed to it is less than IB_RATE_2_5_GBPS or greater than IB_RATE_800_GBPS. Because of this, the code may invoke undefined behaviour when shifting a signed negative value when doing "-EINVAL << 4". To fix this check for errors before assigning stat_rate_sl and propagate any error value to the callers. Signed-off-by: Qasim Ijaz <qasdev00@gmail.com> Fixes: c534ffda781f ("RDMA/mlx5: Fix AH static rate parsing") Cc: stable@vger.kernel.org --- drivers/infiniband/hw/mlx5/ah.c | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-)