[PATCH 1/5] accel/qaic: tighten bounds checking in encode_message()

[Dan Carpenter]

On Thu, Jun 22, 2023 at 04:54:03PM +0530, Pranjal Ramajor Asha Kanojiya wrote:
> 
> 
> On 6/21/2023 12:51 PM, Dan Carpenter wrote:
> > There are several issues in this code.  The check at the start of the
> > loop:
> > 
> > 	if (user_len >= user_msg->len) {
> > 
> > This check does not ensure that we have enough space for the trans_hdr
> > (8 bytes).  Instead the check needs to be:
> > 
> > 	if (user_len >= user_msg->len - sizeof(*trans_hdr)) {
> > 
> > That subtraction is done as an unsigned long we want to avoid
> > negatives.  Add a lower bound to the start of the function.
> > 
> > 	if (user_msg->len < sizeof(*trans_hdr))
> > 
> > There is a second integer underflow which can happen if
> > trans_hdr->len is zero inside the encode_passthrough() function.
> > 
> > 	memcpy(out_trans->data, in_trans->data, in_trans->hdr.len - sizeof(in_trans->hdr));
> > 
> > Instead of adding a check to encode_passthrough() it's better to check
> > in this central place.  Add that check:
> > 
> > 	if (trans_hdr->len < sizeof(trans_hdr)
> > 
> > The final concern is that the "user_len + trans_hdr->len" might have an
> > integer overflow bug.  Use size_add() to prevent that.
> > 
> > -	if (user_len + trans_hdr->len > user_msg->len) {
> > +	if (size_add(user_len, trans_hdr->len) > user_msg->len) {
> > 
> > Fixes: 129776ac2e38 ("accel/qaic: Add control path")
> > Signed-off-by: Dan Carpenter <dan.carpenter at linaro.org>
> > ---
> > This is based on code review and not tested.
> > 
> >   drivers/accel/qaic/qaic_control.c | 8 +++++---
> >   1 file changed, 5 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/accel/qaic/qaic_control.c b/drivers/accel/qaic/qaic_control.c
> > index 5c57f7b4494e..a51b1594dcfa 100644
> > --- a/drivers/accel/qaic/qaic_control.c
> > +++ b/drivers/accel/qaic/qaic_control.c
> > @@ -748,7 +748,8 @@ static int encode_message(struct qaic_device *qdev, struct manage_msg *user_msg,
> >   	int ret;
> >   	int i;
> > -	if (!user_msg->count) {
> > +	if (!user_msg->count ||
> > +	    user_msg->len < sizeof(*trans_hdr)) {
> Can we have something like this here
> user_msg->len < sizeof(*trans_hdr) * user_msg->count, no?

This check was just to ensure that we have space for one header so that
the "user_msg->len - sizeof(*trans_hdr)" subtraction doesn't overflow.
We're going to need to check that we have space for each header later
anyway.  Can the multiply fail (on 32bit)?

> >   		ret = -EINVAL;
> >   		goto out;
> >   	}
> > @@ -765,12 +766,13 @@ static int encode_message(struct qaic_device *qdev, struct manage_msg *user_msg,
> >   	}
> >   	for (i = 0; i < user_msg->count; ++i) {
> > -		if (user_len >= user_msg->len) {
> > +		if (user_len >= user_msg->len - sizeof(*trans_hdr)) {
> Do you think it is more readable if we have something like this
> user_len + sizeof(*trans_hdr) >= user_msg->len

Either way works.  The math should be on trusted side, and to me the
form is always if (variable >= trusted value) { so I prefer to put the
math on right.  But here both sides are trusted and there is no risk of
integer overflow.  If we did that then we could remove the

	if (user_msg->len < sizeof(*trans_hdr))

condition from the start.

regards,
dan carpenter