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

[Dan Carpenter <dan.carpenter@linaro.org>]

On Tue, Jul 04, 2023 at 11:57:51AM +0530, Pranjal Ramajor Asha Kanojiya wrote:
> > 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)) {
> >   		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)) {
> If I understand correctly this check is added to verify if we are left with
> trans_hdr size of data. In that case '>' comparison operator should be used.

That was there in the original code and I thought about changing it but
I don't like changing things which aren't necessary and == is also
invalid so I decided to leave it.

> 
> >   			ret = -EINVAL;
> >   			break;
> >   		}
> >   		trans_hdr = (struct qaic_manage_trans_hdr *)(user_msg->data + user_len);
> > -		if (user_len + trans_hdr->len > user_msg->len) {
> > +		if (trans_hdr->len < sizeof(trans_hdr) ||
> > +		    size_add(user_len, trans_hdr->len) > user_msg->len) {

If we change to > then the == will be caught by this check.  So it
doesn't affect runtime either way.

regards,
dan carpenter