| Message ID | Y9JH5sudiZWvbODv@kili (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | bus: mhi: ep: Fix off by one in mhi_ep_process_cmd_ring() | expand |
On 1/26/23 3:29 AM, Dan Carpenter wrote: > The > comparison should be changed to >= to prevent an out of bounds > access into the mhi_cntrl->mhi_chan[] array. The mhi_cntrl->mhi_chan[] > array is allocated in mhi_ep_chan_init() and has mhi_cntrl->max_chan > elements. You're right. I scanned through that file and there are other spots that don't check that the channel ID is in range, though I think this is the one where it's reading it from an external source. I.e., the other places are aleady known to be correct. (Maybe Mani can comment.) Reviewed-by: Alex Elder <elder@linaro.org> > > Fixes: 2527ad44ddb2 ("bus: mhi: ep: Check if the channel is supported by the controller") > Signed-off-by: Dan Carpenter <error27@gmail.com> > --- > drivers/bus/mhi/ep/main.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/bus/mhi/ep/main.c b/drivers/bus/mhi/ep/main.c > index bcaaba97ef63..be2d56e7f392 100644 > --- a/drivers/bus/mhi/ep/main.c > +++ b/drivers/bus/mhi/ep/main.c > @@ -125,7 +125,7 @@ static int mhi_ep_process_cmd_ring(struct mhi_ep_ring *ring, struct mhi_ring_ele > ch_id = MHI_TRE_GET_CMD_CHID(el); > > /* Check if the channel is supported by the controller */ > - if ((ch_id > mhi_cntrl->max_chan) || !mhi_cntrl->mhi_chan[ch_id].name) { > + if ((ch_id >= mhi_cntrl->max_chan) || !mhi_cntrl->mhi_chan[ch_id].name) { > dev_err(dev, "Channel (%u) not supported!\n", ch_id); > return -ENODEV; > }
On Thu, Jan 26, 2023 at 12:29:10PM +0300, Dan Carpenter wrote: > The > comparison should be changed to >= to prevent an out of bounds > access into the mhi_cntrl->mhi_chan[] array. The mhi_cntrl->mhi_chan[] > array is allocated in mhi_ep_chan_init() and has mhi_cntrl->max_chan > elements. > > Fixes: 2527ad44ddb2 ("bus: mhi: ep: Check if the channel is supported by the controller") > Signed-off-by: Dan Carpenter <error27@gmail.com> Reviewed-by: Manivannan Sadhasivam <mani@kernel.org> Thanks, Mani > --- > drivers/bus/mhi/ep/main.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/bus/mhi/ep/main.c b/drivers/bus/mhi/ep/main.c > index bcaaba97ef63..be2d56e7f392 100644 > --- a/drivers/bus/mhi/ep/main.c > +++ b/drivers/bus/mhi/ep/main.c > @@ -125,7 +125,7 @@ static int mhi_ep_process_cmd_ring(struct mhi_ep_ring *ring, struct mhi_ring_ele > ch_id = MHI_TRE_GET_CMD_CHID(el); > > /* Check if the channel is supported by the controller */ > - if ((ch_id > mhi_cntrl->max_chan) || !mhi_cntrl->mhi_chan[ch_id].name) { > + if ((ch_id >= mhi_cntrl->max_chan) || !mhi_cntrl->mhi_chan[ch_id].name) { > dev_err(dev, "Channel (%u) not supported!\n", ch_id); > return -ENODEV; > } > -- > 2.35.1 >
On Thu, Jan 26, 2023 at 07:26:32AM -0600, Alex Elder wrote: > On 1/26/23 3:29 AM, Dan Carpenter wrote: > > The > comparison should be changed to >= to prevent an out of bounds > > access into the mhi_cntrl->mhi_chan[] array. The mhi_cntrl->mhi_chan[] > > array is allocated in mhi_ep_chan_init() and has mhi_cntrl->max_chan > > elements. > > You're right. I scanned through that file and there > are other spots that don't check that the channel ID > is in range, though I think this is the one where it's > reading it from an external source. I.e., the other > places are aleady known to be correct. (Maybe Mani > can comment.) > Right. This is the only place we get the channel id from the host, so that's why the check is needed only here. > Reviewed-by: Alex Elder <elder@linaro.org> Thanks, Mani > > > > > Fixes: 2527ad44ddb2 ("bus: mhi: ep: Check if the channel is supported by the controller") > > Signed-off-by: Dan Carpenter <error27@gmail.com> > > --- > > drivers/bus/mhi/ep/main.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/bus/mhi/ep/main.c b/drivers/bus/mhi/ep/main.c > > index bcaaba97ef63..be2d56e7f392 100644 > > --- a/drivers/bus/mhi/ep/main.c > > +++ b/drivers/bus/mhi/ep/main.c > > @@ -125,7 +125,7 @@ static int mhi_ep_process_cmd_ring(struct mhi_ep_ring *ring, struct mhi_ring_ele > > ch_id = MHI_TRE_GET_CMD_CHID(el); > > /* Check if the channel is supported by the controller */ > > - if ((ch_id > mhi_cntrl->max_chan) || !mhi_cntrl->mhi_chan[ch_id].name) { > > + if ((ch_id >= mhi_cntrl->max_chan) || !mhi_cntrl->mhi_chan[ch_id].name) { > > dev_err(dev, "Channel (%u) not supported!\n", ch_id); > > return -ENODEV; > > } >
On Thu, Jan 26, 2023 at 12:29:10PM +0300, Dan Carpenter wrote: > The > comparison should be changed to >= to prevent an out of bounds > access into the mhi_cntrl->mhi_chan[] array. The mhi_cntrl->mhi_chan[] > array is allocated in mhi_ep_chan_init() and has mhi_cntrl->max_chan > elements. > > Fixes: 2527ad44ddb2 ("bus: mhi: ep: Check if the channel is supported by the controller") > Signed-off-by: Dan Carpenter <error27@gmail.com> Applied to mhi-next! Thanks, Mani > --- > drivers/bus/mhi/ep/main.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/bus/mhi/ep/main.c b/drivers/bus/mhi/ep/main.c > index bcaaba97ef63..be2d56e7f392 100644 > --- a/drivers/bus/mhi/ep/main.c > +++ b/drivers/bus/mhi/ep/main.c > @@ -125,7 +125,7 @@ static int mhi_ep_process_cmd_ring(struct mhi_ep_ring *ring, struct mhi_ring_ele > ch_id = MHI_TRE_GET_CMD_CHID(el); > > /* Check if the channel is supported by the controller */ > - if ((ch_id > mhi_cntrl->max_chan) || !mhi_cntrl->mhi_chan[ch_id].name) { > + if ((ch_id >= mhi_cntrl->max_chan) || !mhi_cntrl->mhi_chan[ch_id].name) { > dev_err(dev, "Channel (%u) not supported!\n", ch_id); > return -ENODEV; > } > -- > 2.35.1 >
diff --git a/drivers/bus/mhi/ep/main.c b/drivers/bus/mhi/ep/main.c index bcaaba97ef63..be2d56e7f392 100644 --- a/drivers/bus/mhi/ep/main.c +++ b/drivers/bus/mhi/ep/main.c @@ -125,7 +125,7 @@ static int mhi_ep_process_cmd_ring(struct mhi_ep_ring *ring, struct mhi_ring_ele ch_id = MHI_TRE_GET_CMD_CHID(el); /* Check if the channel is supported by the controller */ - if ((ch_id > mhi_cntrl->max_chan) || !mhi_cntrl->mhi_chan[ch_id].name) { + if ((ch_id >= mhi_cntrl->max_chan) || !mhi_cntrl->mhi_chan[ch_id].name) { dev_err(dev, "Channel (%u) not supported!\n", ch_id); return -ENODEV; }
The > comparison should be changed to >= to prevent an out of bounds access into the mhi_cntrl->mhi_chan[] array. The mhi_cntrl->mhi_chan[] array is allocated in mhi_ep_chan_init() and has mhi_cntrl->max_chan elements. Fixes: 2527ad44ddb2 ("bus: mhi: ep: Check if the channel is supported by the controller") Signed-off-by: Dan Carpenter <error27@gmail.com> --- drivers/bus/mhi/ep/main.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)