diff mbox series

[06/10] libpdbg: Return if invalid register numbers are given

Message ID 20180531052915.31171-6-rashmica.g@gmail.com
State Changes Requested
Headers show
Series [01/10] libpdbg: Print the name of the instruction when erroring | expand

Commit Message

Rashmica Gupta May 31, 2018, 5:29 a.m. UTC 
Currently if an invalid register number (eg 44) is given to the
opcode functions an error is printed and the instruction with
the invalid register number is returned. This means that this
invalid instruction is rammed into the machine and causes
the machine to reboot.

So if an invalid register number is supplied, exit out.

Signed-off-by: Rashmica Gupta <rashmica.g@gmail.com>
---
 libpdbg/chip.c | 46 +++++++++++++++++++++++++++++++++++-----------
 1 file changed, 35 insertions(+), 11 deletions(-)

Comments

Alistair Popple June 15, 2018, 1:33 a.m. UTC | #1 
On Thursday, 31 May 2018 3:29:11 PM AEST Rashmica Gupta wrote:
> Currently if an invalid register number (eg 44) is given to the
> opcode functions an error is printed and the instruction with
> the invalid register number is returned. This means that this
> invalid instruction is rammed into the machine and causes
> the machine to reboot.
> 
> So if an invalid register number is supplied, exit out.

Sorry, but I'm not sure I like the idea of just exiting out from the library in
the case of user error. Could we check the register numbers are valid in the
ram_*() instructions and return an error there instead? Or perhaps check the
opcode return value prior to ramming the instructions.

- Alistair

>
> Signed-off-by: Rashmica Gupta <rashmica.g@gmail.com>
> ---
>  libpdbg/chip.c | 46 +++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 35 insertions(+), 11 deletions(-)
> 
> diff --git a/libpdbg/chip.c b/libpdbg/chip.c
> index f478526..b698bcd 100644
> --- a/libpdbg/chip.c
> +++ b/libpdbg/chip.c
> @@ -28,66 +28,84 @@
>  
>  static uint64_t mfspr(uint64_t reg, uint64_t spr)
>  {
> -	if (reg > 31)
> +	if (reg > 31) {
>  		PR_ERROR("Invalid register specified for mfspr\n");
> +		exit(1);
> +	}
>  
>  	return MFSPR_OPCODE | (reg << 21) | ((spr & 0x1f) << 16) | ((spr & 0x3e0) << 6);
>  }
>  
>  static uint64_t mtspr(uint64_t spr, uint64_t reg)
>  {
> -	if (reg > 31)
> +	if (reg > 31) {
>  		PR_ERROR("Invalid register specified for mtspr\n");
> +		exit(1);
> +	}
>  
>  	return MTSPR_OPCODE | (reg << 21) | ((spr & 0x1f) << 16) | ((spr & 0x3e0) << 6);
>  }
>  
>  static uint64_t mfocrf(uint64_t reg, uint64_t cr)
>  {
> -	if (reg > 31)
> +	if (reg > 31) {
>  		PR_ERROR("Invalid register specified for mfocrf\n");
> -	if (cr > 7)
> +		exit(1);
> +	}
> +	if (cr > 7) {
>  		PR_ERROR("Invalid CR field specified\n");
> +		exit(1);
> +	}
>  
>  	return MFOCRF_OPCODE | (reg << 21) | (1U << (12 + cr));
>  }
>  
>  static uint64_t mfnia(uint64_t reg)
>  {
> -	if (reg > 31)
> +	if (reg > 31) {
>  		PR_ERROR("Invalid register specified for mfnia\n");
> +		exit(1);
> +	}
>  
>  	return MFNIA_OPCODE | (reg << 21);
>  }
>  
>  static uint64_t mtnia(uint64_t reg)
>  {
> -	if (reg > 31)
> +	if (reg > 31) {
>  		PR_ERROR("Invalid register specified for mtnia\n");
> +		exit(1);
> +	}
>  
>  	return MTNIA_OPCODE | (reg << 21);
>  }
>  
>  static uint64_t mfmsr(uint64_t reg)
>  {
> -	if (reg > 31)
> +	if (reg > 31) {
>  		PR_ERROR("Invalid register specified for mfmsr\n");
> +		exit(1);
> +	}
>  
>  	return MFMSR_OPCODE | (reg << 21);
>  }
>  
>  static uint64_t mtmsr(uint64_t reg)
>  {
> -	if (reg > 31)
> +	if (reg > 31) {
>  		PR_ERROR("Invalid register specified for mtmsr\n");
> +		exit(1);
> +	}
>  
>  	return MTMSR_OPCODE | (reg << 21);
>  }
>  
>  static uint64_t mfxerf(uint64_t reg, uint64_t field)
>  {
> -	if (reg > 31)
> +	if (reg > 31) {
>  		PR_ERROR("Invalid register specified for mfxerf\n");
> +		exit(1);
> +	}
>  
>  	switch(field) {
>  	case 0:
> @@ -100,14 +118,17 @@ static uint64_t mfxerf(uint64_t reg, uint64_t field)
>  		return MFXERF3_OPCODE | (reg << 21);
>  	default:
>  		PR_ERROR("Invalid XER field specified\n");
> +		exit(1);
>  	}
>  	return 0;
>  }
>  
>  static uint64_t mtxerf(uint64_t reg, uint64_t field)
>  {
> -	if (reg > 31)
> +	if (reg > 31) {
>  		PR_ERROR("Invalid register specified for mtxerf\n");
> +		exit(1);
> +	}
>  
>  	switch(field) {
>  	case 0:
> @@ -120,14 +141,17 @@ static uint64_t mtxerf(uint64_t reg, uint64_t field)
>  		return MTXERF3_OPCODE | (reg << 21);
>  	default:
>  		PR_ERROR("Invalid XER field specified\n");
> +		exit(1);
>  	}
>  	return 0;
>  }
>  
>  static uint64_t ld(uint64_t rt, uint64_t ds, uint64_t ra)
>  {
> -	if ((rt > 31) | (ra > 31) | (ds > 0x3fff))
> +	if ((rt > 31) | (ra > 31) | (ds > 0x3fff)) {
>  		PR_ERROR("Invalid register specified\n");
> +		exit(1);
> +	}
>  
>  	return LD_OPCODE | (rt << 21) | (ra << 16) | (ds << 2);
>  }
>
Rashmica Gupta June 15, 2018, 6:22 a.m. UTC | #2 
On 15/06/18 11:33, Alistair Popple wrote:
> On Thursday, 31 May 2018 3:29:11 PM AEST Rashmica Gupta wrote:
>> Currently if an invalid register number (eg 44) is given to the
>> opcode functions an error is printed and the instruction with
>> the invalid register number is returned. This means that this
>> invalid instruction is rammed into the machine and causes
>> the machine to reboot.
>>
>> So if an invalid register number is supplied, exit out.
> Sorry, but I'm not sure I like the idea of just exiting out from the library in
> the case of user error. Could we check the register numbers are valid in the
> ram_*() instructions and return an error there instead? Or perhaps check the
> opcode return value prior to ramming the instructions.
Or return INVALID_OPCODE where INVALID_OPCODE is defined with the other
opcodes?
>
> - Alistair
>
>> Signed-off-by: Rashmica Gupta <rashmica.g@gmail.com>
>> ---
>>  libpdbg/chip.c | 46 +++++++++++++++++++++++++++++++++++-----------
>>  1 file changed, 35 insertions(+), 11 deletions(-)
>>
>> diff --git a/libpdbg/chip.c b/libpdbg/chip.c
>> index f478526..b698bcd 100644
>> --- a/libpdbg/chip.c
>> +++ b/libpdbg/chip.c
>> @@ -28,66 +28,84 @@
>>  
>>  static uint64_t mfspr(uint64_t reg, uint64_t spr)
>>  {
>> -	if (reg > 31)
>> +	if (reg > 31) {
>>  		PR_ERROR("Invalid register specified for mfspr\n");
>> +		exit(1);
>> +	}
>>  
>>  	return MFSPR_OPCODE | (reg << 21) | ((spr & 0x1f) << 16) | ((spr & 0x3e0) << 6);
>>  }
>>  
>>  static uint64_t mtspr(uint64_t spr, uint64_t reg)
>>  {
>> -	if (reg > 31)
>> +	if (reg > 31) {
>>  		PR_ERROR("Invalid register specified for mtspr\n");
>> +		exit(1);
>> +	}
>>  
>>  	return MTSPR_OPCODE | (reg << 21) | ((spr & 0x1f) << 16) | ((spr & 0x3e0) << 6);
>>  }
>>  
>>  static uint64_t mfocrf(uint64_t reg, uint64_t cr)
>>  {
>> -	if (reg > 31)
>> +	if (reg > 31) {
>>  		PR_ERROR("Invalid register specified for mfocrf\n");
>> -	if (cr > 7)
>> +		exit(1);
>> +	}
>> +	if (cr > 7) {
>>  		PR_ERROR("Invalid CR field specified\n");
>> +		exit(1);
>> +	}
>>  
>>  	return MFOCRF_OPCODE | (reg << 21) | (1U << (12 + cr));
>>  }
>>  
>>  static uint64_t mfnia(uint64_t reg)
>>  {
>> -	if (reg > 31)
>> +	if (reg > 31) {
>>  		PR_ERROR("Invalid register specified for mfnia\n");
>> +		exit(1);
>> +	}
>>  
>>  	return MFNIA_OPCODE | (reg << 21);
>>  }
>>  
>>  static uint64_t mtnia(uint64_t reg)
>>  {
>> -	if (reg > 31)
>> +	if (reg > 31) {
>>  		PR_ERROR("Invalid register specified for mtnia\n");
>> +		exit(1);
>> +	}
>>  
>>  	return MTNIA_OPCODE | (reg << 21);
>>  }
>>  
>>  static uint64_t mfmsr(uint64_t reg)
>>  {
>> -	if (reg > 31)
>> +	if (reg > 31) {
>>  		PR_ERROR("Invalid register specified for mfmsr\n");
>> +		exit(1);
>> +	}
>>  
>>  	return MFMSR_OPCODE | (reg << 21);
>>  }
>>  
>>  static uint64_t mtmsr(uint64_t reg)
>>  {
>> -	if (reg > 31)
>> +	if (reg > 31) {
>>  		PR_ERROR("Invalid register specified for mtmsr\n");
>> +		exit(1);
>> +	}
>>  
>>  	return MTMSR_OPCODE | (reg << 21);
>>  }
>>  
>>  static uint64_t mfxerf(uint64_t reg, uint64_t field)
>>  {
>> -	if (reg > 31)
>> +	if (reg > 31) {
>>  		PR_ERROR("Invalid register specified for mfxerf\n");
>> +		exit(1);
>> +	}
>>  
>>  	switch(field) {
>>  	case 0:
>> @@ -100,14 +118,17 @@ static uint64_t mfxerf(uint64_t reg, uint64_t field)
>>  		return MFXERF3_OPCODE | (reg << 21);
>>  	default:
>>  		PR_ERROR("Invalid XER field specified\n");
>> +		exit(1);
>>  	}
>>  	return 0;
>>  }
>>  
>>  static uint64_t mtxerf(uint64_t reg, uint64_t field)
>>  {
>> -	if (reg > 31)
>> +	if (reg > 31) {
>>  		PR_ERROR("Invalid register specified for mtxerf\n");
>> +		exit(1);
>> +	}
>>  
>>  	switch(field) {
>>  	case 0:
>> @@ -120,14 +141,17 @@ static uint64_t mtxerf(uint64_t reg, uint64_t field)
>>  		return MTXERF3_OPCODE | (reg << 21);
>>  	default:
>>  		PR_ERROR("Invalid XER field specified\n");
>> +		exit(1);
>>  	}
>>  	return 0;
>>  }
>>  
>>  static uint64_t ld(uint64_t rt, uint64_t ds, uint64_t ra)
>>  {
>> -	if ((rt > 31) | (ra > 31) | (ds > 0x3fff))
>> +	if ((rt > 31) | (ra > 31) | (ds > 0x3fff)) {
>>  		PR_ERROR("Invalid register specified\n");
>> +		exit(1);
>> +	}
>>  
>>  	return LD_OPCODE | (rt << 21) | (ra << 16) | (ds << 2);
>>  }
>>
>
Alistair Popple June 15, 2018, 6:59 a.m. UTC | #3 
On Friday, 15 June 2018 4:22:28 PM AEST rashmica wrote:
> 
> On 15/06/18 11:33, Alistair Popple wrote:
> > On Thursday, 31 May 2018 3:29:11 PM AEST Rashmica Gupta wrote:
> >> Currently if an invalid register number (eg 44) is given to the
> >> opcode functions an error is printed and the instruction with
> >> the invalid register number is returned. This means that this
> >> invalid instruction is rammed into the machine and causes
> >> the machine to reboot.
> >>
> >> So if an invalid register number is supplied, exit out.
> > Sorry, but I'm not sure I like the idea of just exiting out from the library in
> > the case of user error. Could we check the register numbers are valid in the
> > ram_*() instructions and return an error there instead? Or perhaps check the
> > opcode return value prior to ramming the instructions.
> Or return INVALID_OPCODE where INVALID_OPCODE is defined with the other
> opcodes?

Yep, that's basically what I was getting at. Although you'd gave to loop over
all the opcodes to check that, although I guess that would be easy enough to do
in ram_instructions() (although I'd rather we didn't even attempt instruction
ramming if user input is invalid so make sure it's a seperate check if you go
that path).

- Alistair

> > - Alistair
> >
> >> Signed-off-by: Rashmica Gupta <rashmica.g@gmail.com>
> >> ---
> >>  libpdbg/chip.c | 46 +++++++++++++++++++++++++++++++++++-----------
> >>  1 file changed, 35 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/libpdbg/chip.c b/libpdbg/chip.c
> >> index f478526..b698bcd 100644
> >> --- a/libpdbg/chip.c
> >> +++ b/libpdbg/chip.c
> >> @@ -28,66 +28,84 @@
> >>  
> >>  static uint64_t mfspr(uint64_t reg, uint64_t spr)
> >>  {
> >> -	if (reg > 31)
> >> +	if (reg > 31) {
> >>  		PR_ERROR("Invalid register specified for mfspr\n");
> >> +		exit(1);
> >> +	}
> >>  
> >>  	return MFSPR_OPCODE | (reg << 21) | ((spr & 0x1f) << 16) | ((spr & 0x3e0) << 6);
> >>  }
> >>  
> >>  static uint64_t mtspr(uint64_t spr, uint64_t reg)
> >>  {
> >> -	if (reg > 31)
> >> +	if (reg > 31) {
> >>  		PR_ERROR("Invalid register specified for mtspr\n");
> >> +		exit(1);
> >> +	}
> >>  
> >>  	return MTSPR_OPCODE | (reg << 21) | ((spr & 0x1f) << 16) | ((spr & 0x3e0) << 6);
> >>  }
> >>  
> >>  static uint64_t mfocrf(uint64_t reg, uint64_t cr)
> >>  {
> >> -	if (reg > 31)
> >> +	if (reg > 31) {
> >>  		PR_ERROR("Invalid register specified for mfocrf\n");
> >> -	if (cr > 7)
> >> +		exit(1);
> >> +	}
> >> +	if (cr > 7) {
> >>  		PR_ERROR("Invalid CR field specified\n");
> >> +		exit(1);
> >> +	}
> >>  
> >>  	return MFOCRF_OPCODE | (reg << 21) | (1U << (12 + cr));
> >>  }
> >>  
> >>  static uint64_t mfnia(uint64_t reg)
> >>  {
> >> -	if (reg > 31)
> >> +	if (reg > 31) {
> >>  		PR_ERROR("Invalid register specified for mfnia\n");
> >> +		exit(1);
> >> +	}
> >>  
> >>  	return MFNIA_OPCODE | (reg << 21);
> >>  }
> >>  
> >>  static uint64_t mtnia(uint64_t reg)
> >>  {
> >> -	if (reg > 31)
> >> +	if (reg > 31) {
> >>  		PR_ERROR("Invalid register specified for mtnia\n");
> >> +		exit(1);
> >> +	}
> >>  
> >>  	return MTNIA_OPCODE | (reg << 21);
> >>  }
> >>  
> >>  static uint64_t mfmsr(uint64_t reg)
> >>  {
> >> -	if (reg > 31)
> >> +	if (reg > 31) {
> >>  		PR_ERROR("Invalid register specified for mfmsr\n");
> >> +		exit(1);
> >> +	}
> >>  
> >>  	return MFMSR_OPCODE | (reg << 21);
> >>  }
> >>  
> >>  static uint64_t mtmsr(uint64_t reg)
> >>  {
> >> -	if (reg > 31)
> >> +	if (reg > 31) {
> >>  		PR_ERROR("Invalid register specified for mtmsr\n");
> >> +		exit(1);
> >> +	}
> >>  
> >>  	return MTMSR_OPCODE | (reg << 21);
> >>  }
> >>  
> >>  static uint64_t mfxerf(uint64_t reg, uint64_t field)
> >>  {
> >> -	if (reg > 31)
> >> +	if (reg > 31) {
> >>  		PR_ERROR("Invalid register specified for mfxerf\n");
> >> +		exit(1);
> >> +	}
> >>  
> >>  	switch(field) {
> >>  	case 0:
> >> @@ -100,14 +118,17 @@ static uint64_t mfxerf(uint64_t reg, uint64_t field)
> >>  		return MFXERF3_OPCODE | (reg << 21);
> >>  	default:
> >>  		PR_ERROR("Invalid XER field specified\n");
> >> +		exit(1);
> >>  	}
> >>  	return 0;
> >>  }
> >>  
> >>  static uint64_t mtxerf(uint64_t reg, uint64_t field)
> >>  {
> >> -	if (reg > 31)
> >> +	if (reg > 31) {
> >>  		PR_ERROR("Invalid register specified for mtxerf\n");
> >> +		exit(1);
> >> +	}
> >>  
> >>  	switch(field) {
> >>  	case 0:
> >> @@ -120,14 +141,17 @@ static uint64_t mtxerf(uint64_t reg, uint64_t field)
> >>  		return MTXERF3_OPCODE | (reg << 21);
> >>  	default:
> >>  		PR_ERROR("Invalid XER field specified\n");
> >> +		exit(1);
> >>  	}
> >>  	return 0;
> >>  }
> >>  
> >>  static uint64_t ld(uint64_t rt, uint64_t ds, uint64_t ra)
> >>  {
> >> -	if ((rt > 31) | (ra > 31) | (ds > 0x3fff))
> >> +	if ((rt > 31) | (ra > 31) | (ds > 0x3fff)) {
> >>  		PR_ERROR("Invalid register specified\n");
> >> +		exit(1);
> >> +	}
> >>  
> >>  	return LD_OPCODE | (rt << 21) | (ra << 16) | (ds << 2);
> >>  }
> >>
> >
> 
>
diff mbox series

Patch

diff --git a/libpdbg/chip.c b/libpdbg/chip.c
index f478526..b698bcd 100644
--- a/libpdbg/chip.c
+++ b/libpdbg/chip.c
@@ -28,66 +28,84 @@ 
 
 static uint64_t mfspr(uint64_t reg, uint64_t spr)
 {
-	if (reg > 31)
+	if (reg > 31) {
 		PR_ERROR("Invalid register specified for mfspr\n");
+		exit(1);
+	}
 
 	return MFSPR_OPCODE | (reg << 21) | ((spr & 0x1f) << 16) | ((spr & 0x3e0) << 6);
 }
 
 static uint64_t mtspr(uint64_t spr, uint64_t reg)
 {
-	if (reg > 31)
+	if (reg > 31) {
 		PR_ERROR("Invalid register specified for mtspr\n");
+		exit(1);
+	}
 
 	return MTSPR_OPCODE | (reg << 21) | ((spr & 0x1f) << 16) | ((spr & 0x3e0) << 6);
 }
 
 static uint64_t mfocrf(uint64_t reg, uint64_t cr)
 {
-	if (reg > 31)
+	if (reg > 31) {
 		PR_ERROR("Invalid register specified for mfocrf\n");
-	if (cr > 7)
+		exit(1);
+	}
+	if (cr > 7) {
 		PR_ERROR("Invalid CR field specified\n");
+		exit(1);
+	}
 
 	return MFOCRF_OPCODE | (reg << 21) | (1U << (12 + cr));
 }
 
 static uint64_t mfnia(uint64_t reg)
 {
-	if (reg > 31)
+	if (reg > 31) {
 		PR_ERROR("Invalid register specified for mfnia\n");
+		exit(1);
+	}
 
 	return MFNIA_OPCODE | (reg << 21);
 }
 
 static uint64_t mtnia(uint64_t reg)
 {
-	if (reg > 31)
+	if (reg > 31) {
 		PR_ERROR("Invalid register specified for mtnia\n");
+		exit(1);
+	}
 
 	return MTNIA_OPCODE | (reg << 21);
 }
 
 static uint64_t mfmsr(uint64_t reg)
 {
-	if (reg > 31)
+	if (reg > 31) {
 		PR_ERROR("Invalid register specified for mfmsr\n");
+		exit(1);
+	}
 
 	return MFMSR_OPCODE | (reg << 21);
 }
 
 static uint64_t mtmsr(uint64_t reg)
 {
-	if (reg > 31)
+	if (reg > 31) {
 		PR_ERROR("Invalid register specified for mtmsr\n");
+		exit(1);
+	}
 
 	return MTMSR_OPCODE | (reg << 21);
 }
 
 static uint64_t mfxerf(uint64_t reg, uint64_t field)
 {
-	if (reg > 31)
+	if (reg > 31) {
 		PR_ERROR("Invalid register specified for mfxerf\n");
+		exit(1);
+	}
 
 	switch(field) {
 	case 0:
@@ -100,14 +118,17 @@  static uint64_t mfxerf(uint64_t reg, uint64_t field)
 		return MFXERF3_OPCODE | (reg << 21);
 	default:
 		PR_ERROR("Invalid XER field specified\n");
+		exit(1);
 	}
 	return 0;
 }
 
 static uint64_t mtxerf(uint64_t reg, uint64_t field)
 {
-	if (reg > 31)
+	if (reg > 31) {
 		PR_ERROR("Invalid register specified for mtxerf\n");
+		exit(1);
+	}
 
 	switch(field) {
 	case 0:
@@ -120,14 +141,17 @@  static uint64_t mtxerf(uint64_t reg, uint64_t field)
 		return MTXERF3_OPCODE | (reg << 21);
 	default:
 		PR_ERROR("Invalid XER field specified\n");
+		exit(1);
 	}
 	return 0;
 }
 
 static uint64_t ld(uint64_t rt, uint64_t ds, uint64_t ra)
 {
-	if ((rt > 31) | (ra > 31) | (ds > 0x3fff))
+	if ((rt > 31) | (ra > 31) | (ds > 0x3fff)) {
 		PR_ERROR("Invalid register specified\n");
+		exit(1);
+	}
 
 	return LD_OPCODE | (rt << 21) | (ra << 16) | (ds << 2);
 }