diff mbox series

drm/i915/guc: Fix potential null pointer deref in GuC 'steal id' test

Message ID 20230802184940.911753-1-John.C.Harrison@Intel.com (mailing list archive)
State New, archived
Headers show
Series drm/i915/guc: Fix potential null pointer deref in GuC 'steal id' test | expand

Commit Message

John Harrison Aug. 2, 2023, 6:49 p.m. UTC 
From: John Harrison <John.C.Harrison@Intel.com>

It was noticed that if the very first 'stealing' request failed to
create for some reason then the 'steal all ids' loop would immediately
exit with 'last' still being NULL. The test would attempt to continue
but using a null pointer. Fix that by aborting the test if it fails to
create any requests at all.

Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
---
 drivers/gpu/drm/i915/gt/uc/selftest_guc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Andi Shyti Aug. 3, 2023, 1:28 p.m. UTC | #1 
Hi John,

On Wed, Aug 02, 2023 at 11:49:40AM -0700, John.C.Harrison@Intel.com wrote:
> From: John Harrison <John.C.Harrison@Intel.com>
> 
> It was noticed that if the very first 'stealing' request failed to
> create for some reason then the 'steal all ids' loop would immediately
> exit with 'last' still being NULL. The test would attempt to continue
> but using a null pointer. Fix that by aborting the test if it fails to
> create any requests at all.
> 
> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
> ---
>  drivers/gpu/drm/i915/gt/uc/selftest_guc.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/uc/selftest_guc.c b/drivers/gpu/drm/i915/gt/uc/selftest_guc.c
> index 1fd760539f77b..bfb72143566f6 100644
> --- a/drivers/gpu/drm/i915/gt/uc/selftest_guc.c
> +++ b/drivers/gpu/drm/i915/gt/uc/selftest_guc.c
> @@ -204,9 +204,9 @@ static int intel_guc_steal_guc_ids(void *arg)
>  		if (IS_ERR(rq)) {
>  			ret = PTR_ERR(rq);
>  			rq = NULL;
> -			if (ret != -EAGAIN) {
> -				guc_err(guc, "Failed to create request %d: %pe\n",
> -					context_index, ERR_PTR(ret));
> +			if ((ret != -EAGAIN) || !last) {

isn't last alway NULL here?

Andi

> +				guc_err(guc, "Failed to create %srequest %d: %pe\n",
> +					last ? "" : "first ", context_index, ERR_PTR(ret));
>  				goto err_spin_rq;
>  			}
>  		} else {
> -- 
> 2.39.1
John Harrison Aug. 7, 2023, 7:46 p.m. UTC | #2 
On 8/3/2023 06:28, Andi Shyti wrote:
> Hi John,
>
> On Wed, Aug 02, 2023 at 11:49:40AM -0700, John.C.Harrison@Intel.com wrote:
>> From: John Harrison <John.C.Harrison@Intel.com>
>>
>> It was noticed that if the very first 'stealing' request failed to
>> create for some reason then the 'steal all ids' loop would immediately
>> exit with 'last' still being NULL. The test would attempt to continue
>> but using a null pointer. Fix that by aborting the test if it fails to
>> create any requests at all.
>>
>> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
>> ---
>>   drivers/gpu/drm/i915/gt/uc/selftest_guc.c | 6 +++---
>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gt/uc/selftest_guc.c b/drivers/gpu/drm/i915/gt/uc/selftest_guc.c
>> index 1fd760539f77b..bfb72143566f6 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/selftest_guc.c
>> +++ b/drivers/gpu/drm/i915/gt/uc/selftest_guc.c
>> @@ -204,9 +204,9 @@ static int intel_guc_steal_guc_ids(void *arg)
>>   		if (IS_ERR(rq)) {
>>   			ret = PTR_ERR(rq);
>>   			rq = NULL;
>> -			if (ret != -EAGAIN) {
>> -				guc_err(guc, "Failed to create request %d: %pe\n",
>> -					context_index, ERR_PTR(ret));
>> +			if ((ret != -EAGAIN) || !last) {
> isn't last alway NULL here?
>
> Andi
No, only on the first pass around the loop. When a request is 
successfully created, the else clause below assigns last to that new 
request. So if the failure to create only happens on pass 2 or later, 
last will be non-null. Which is the whole point of the code. It keeps 
creating all the contexts/requests that it can until it runs out of 
resources and gets an EAGAIN failure. At which point, last will be 
pointing to the last successful creation and the test continues to the 
next part of actually stealing an id.

But if the EAGAIN failure happens on the first pass then last will be 
null and it is not safe/valid to proceed so it needs to abort. And if 
anything other than EAGAIN is returned then something has gone wrong and 
it doesn't matter what last is set to, it needs to abort regardless.

John.


>
>> +				guc_err(guc, "Failed to create %srequest %d: %pe\n",
>> +					last ? "" : "first ", context_index, ERR_PTR(ret));
>>   				goto err_spin_rq;
>>   			}
>>   		} else {
>> -- 
>> 2.39.1
Andi Shyti Aug. 10, 2023, 9:50 a.m. UTC | #3 
On Mon, Aug 07, 2023 at 12:46:46PM -0700, John Harrison wrote:
> On 8/3/2023 06:28, Andi Shyti wrote:
> > Hi John,
> > 
> > On Wed, Aug 02, 2023 at 11:49:40AM -0700, John.C.Harrison@Intel.com wrote:
> > > From: John Harrison <John.C.Harrison@Intel.com>
> > > 
> > > It was noticed that if the very first 'stealing' request failed to
> > > create for some reason then the 'steal all ids' loop would immediately
> > > exit with 'last' still being NULL. The test would attempt to continue
> > > but using a null pointer. Fix that by aborting the test if it fails to
> > > create any requests at all.
> > > 
> > > Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
> > > ---
> > >   drivers/gpu/drm/i915/gt/uc/selftest_guc.c | 6 +++---
> > >   1 file changed, 3 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/gt/uc/selftest_guc.c b/drivers/gpu/drm/i915/gt/uc/selftest_guc.c
> > > index 1fd760539f77b..bfb72143566f6 100644
> > > --- a/drivers/gpu/drm/i915/gt/uc/selftest_guc.c
> > > +++ b/drivers/gpu/drm/i915/gt/uc/selftest_guc.c
> > > @@ -204,9 +204,9 @@ static int intel_guc_steal_guc_ids(void *arg)
> > >   		if (IS_ERR(rq)) {
> > >   			ret = PTR_ERR(rq);
> > >   			rq = NULL;
> > > -			if (ret != -EAGAIN) {
> > > -				guc_err(guc, "Failed to create request %d: %pe\n",
> > > -					context_index, ERR_PTR(ret));
> > > +			if ((ret != -EAGAIN) || !last) {
> > isn't last alway NULL here?
> > 
> > Andi
> No, only on the first pass around the loop. When a request is successfully
> created, the else clause below assigns last to that new request. So if the
> failure to create only happens on pass 2 or later, last will be non-null.
> Which is the whole point of the code. It keeps creating all the
> contexts/requests that it can until it runs out of resources and gets an
> EAGAIN failure. At which point, last will be pointing to the last successful
> creation and the test continues to the next part of actually stealing an id.
> 
> But if the EAGAIN failure happens on the first pass then last will be null
> and it is not safe/valid to proceed so it needs to abort. And if anything
> other than EAGAIN is returned then something has gone wrong and it doesn't
> matter what last is set to, it needs to abort regardless.

Right! Thanks!

Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com> 

Andi
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/gt/uc/selftest_guc.c b/drivers/gpu/drm/i915/gt/uc/selftest_guc.c
index 1fd760539f77b..bfb72143566f6 100644
--- a/drivers/gpu/drm/i915/gt/uc/selftest_guc.c
+++ b/drivers/gpu/drm/i915/gt/uc/selftest_guc.c
@@ -204,9 +204,9 @@  static int intel_guc_steal_guc_ids(void *arg)
 		if (IS_ERR(rq)) {
 			ret = PTR_ERR(rq);
 			rq = NULL;
-			if (ret != -EAGAIN) {
-				guc_err(guc, "Failed to create request %d: %pe\n",
-					context_index, ERR_PTR(ret));
+			if ((ret != -EAGAIN) || !last) {
+				guc_err(guc, "Failed to create %srequest %d: %pe\n",
+					last ? "" : "first ", context_index, ERR_PTR(ret));
 				goto err_spin_rq;
 			}
 		} else {