[PATCH v2 0/2] opcodes/nfp: bug fix for nfp disassembler

[PATCH v2 0/2] opcodes/nfp: bug fix for nfp disassembler

[Yinjun Zhang]

This series is to fix some bugs in nfp disassembler.

Yinjun Zhang (2):
  opcodes/nfp: add validity check of island and me
  opcodes/nfp: skip those non-code sections

 opcodes/nfp-dis.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

-- 
2.27.0

[PATCH v2 1/2] opcodes/nfp: add validity check of island and me

[Yinjun Zhang]

AddressSanitizer detects heap-buffer-overflow when running
"objdump -D" for nfp .nffw files.

Add necessary check for parsed island and me number.

Signed-off-by: Yinjun Zhang <yinjun.zhang@corigine.com>
Signed-off-by: Simon Horman <simon.horman@corigine.com>
---
 opcodes/nfp-dis.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/opcodes/nfp-dis.c b/opcodes/nfp-dis.c
index b74ccb3fce5..ff69410d967 100644
--- a/opcodes/nfp-dis.c
+++ b/opcodes/nfp-dis.c
@@ -46,6 +46,9 @@
 #define _NFP_ME27_28_CSR_CTX_ENABLES     0x18
 #define _NFP_ME27_28_CSR_MISC_CONTROL    0x160
 
+#define _NFP_ISLAND_MAX 64
+#define _NFP_ME_MAX     12
+
 typedef struct
 {
   unsigned char ctx4_mode:1;
@@ -65,7 +68,7 @@ nfp_opts;
 /* mecfgs[island][menum][is-text] */
 typedef struct
 {
-  nfp_priv_mecfg mecfgs[64][12][2];
+  nfp_priv_mecfg mecfgs[_NFP_ISLAND_MAX][_NFP_ME_MAX][2];
 }
 nfp_priv_data;
 
@@ -2837,6 +2840,12 @@ _print_instrs (bfd_vma addr, struct disassemble_info *dinfo, nfp_opts * opts)
 	  break;
 	}
 
+      if ((island >= _NFP_ISLAND_MAX) || (menum >= _NFP_ME_MAX))
+	{
+	  dinfo->fprintf_func (dinfo->stream, "Invalid island or me.");
+	  return _NFP_ERR_STOP;
+	}
+
       mecfg = &priv->mecfgs[island][menum][is_text];
       num_ctx = (mecfg->ctx4_mode) ? 4 : 8;
       addr_3rdparty32 = mecfg->addr_3rdparty32;
-- 
2.27.0

[PATCH v2 2/2] opcodes/nfp: skip those non-code sections

[Yinjun Zhang]

Currently nfp disassemblers can only support code section,
and we don't require to disassmeble other sections for now.
So skip the non-code sections.

Signed-off-by: Yinjun Zhang <yinjun.zhang@corigine.com>
Signed-off-by: Simon Horman <simon.horman@corigine.com>
---
 opcodes/nfp-dis.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/opcodes/nfp-dis.c b/opcodes/nfp-dis.c
index ff69410d967..05f7a27bc46 100644
--- a/opcodes/nfp-dis.c
+++ b/opcodes/nfp-dis.c
@@ -2959,6 +2959,14 @@ print_insn_nfp (bfd_vma addr, struct disassemble_info *dinfo)
   nfp_opts opts;
   int err;
 
+  /* Currently only disassemble the text section.  */
+  if (!(dinfo->section->flags & SEC_CODE))
+    {
+      dinfo->fprintf_func (dinfo->stream,
+			   "Disassembly of this section is not supported\t # SKIP");
+      return -1;
+    }
+
   opts.show_pc = 1;
   opts.ctx_mode = 0;
   err = parse_disassembler_options (&opts, dinfo);
-- 
2.27.0

Re: [PATCH v2 0/2] opcodes/nfp: bug fix for nfp disassembler

[Alan Modra]

On Wed, Aug 25, 2021 at 09:01:16PM -0400, Yinjun Zhang wrote:
> This series is to fix some bugs in nfp disassembler.
> 
> Yinjun Zhang (2):
>   opcodes/nfp: add validity check of island and me

Thanks, I applied this one,

>   opcodes/nfp: skip those non-code sections

but won't apply this.  Disabling objdump -D is not a solution to bugs
found by fuzzers, because a very simple change to the attack object
will result in the same bug being exposed with objdump -d.  The whole
point of objdump -D is to disassemble non-code, knowing that it is
likely to result in nonsense.

By the way, you have another similar problem in init_nfp6000_mecsr_sec
with the menum calculation from a bit-field read from an object file.
That also needs to be sanity checked.  Bit-field values of 0 to 3 in
the file will result in out of bounds mecfgs array access.

-- 
Alan Modra
Australia Development Lab, IBM

RE: [PATCH v2 0/2] opcodes/nfp: bug fix for nfp disassembler

[Yinjun Zhang]

+ Simon Horman in the loop.

> -----Original Message-----
> From: Alan Modra <amodra@gmail.com>
> Sent: Wednesday, September 1, 2021 9:25 AM
> To: Yinjun Zhang <yinjun.zhang@corigine.com>
> Cc: binutils@sourceware.org
> Subject: Re: [PATCH v2 0/2] opcodes/nfp: bug fix for nfp disassembler
> 
> On Wed, Aug 25, 2021 at 09:01:16PM -0400, Yinjun Zhang wrote:
> > This series is to fix some bugs in nfp disassembler.
> >
> > Yinjun Zhang (2):
> >   opcodes/nfp: add validity check of island and me
> 
> Thanks, I applied this one,

Thanks.

> 
> >   opcodes/nfp: skip those non-code sections
> 
> but won't apply this.  Disabling objdump -D is not a solution to bugs found by
> fuzzers, because a very simple change to the attack object will result in the
> same bug being exposed with objdump -d.  

Yes, you're right, and thanks for pointing this out, and I think that's why the previous
commit is needed and applied.

> The whole point of objdump -D is
> to disassemble non-code, knowing that it is likely to result in nonsense.

I understand "objdump -D" is to disassemble all sections, including those non-code
ones, however current nfp disassembly code is not prepared for that. Disassembling
those sections just result in meaningless output, so I propose to skip those sections
for now.

> 
> By the way, you have another similar problem in init_nfp6000_mecsr_sec
> with the menum calculation from a bit-field read from an object file.
> That also needs to be sanity checked.  Bit-field values of 0 to 3 in the file will
> result in out of bounds mecfgs array access.

I'd checked this part, you mean "menum = _BF (ireg.cpp_offset_lo, 13, 10) - 4", 
right? There's a "-4", so I think it's safe.

> 
> --
> Alan Modra
> Australia Development Lab, IBM

Re: [PATCH v2 0/2] opcodes/nfp: bug fix for nfp disassembler

[Alan Modra]

On Wed, Sep 01, 2021 at 07:12:27AM +0000, Yinjun Zhang wrote:
> > By the way, you have another similar problem in init_nfp6000_mecsr_sec
> > with the menum calculation from a bit-field read from an object file.
> > That also needs to be sanity checked.  Bit-field values of 0 to 3 in the file will
> > result in out of bounds mecfgs array access.
> 
> I'd checked this part, you mean "menum = _BF (ireg.cpp_offset_lo, 13, 10) - 4", 
> right? There's a "-4", so I think it's safe.

If the bit-field is 0, which it might be with a fuzzed object, then
menum == (size_t) -4.  That is out of bounds.

-- 
Alan Modra
Australia Development Lab, IBM

RE: [PATCH v2 0/2] opcodes/nfp: bug fix for nfp disassembler

[Yinjun Zhang]

> -----Original Message-----
> From: Alan Modra <amodra@gmail.com>
> Sent: Wednesday, September 1, 2021 6:02 PM
> To: Yinjun Zhang <yinjun.zhang@corigine.com>
> Cc: binutils@sourceware.org; Simon Horman <simon.horman@corigine.com>
> Subject: Re: [PATCH v2 0/2] opcodes/nfp: bug fix for nfp disassembler
> 
> On Wed, Sep 01, 2021 at 07:12:27AM +0000, Yinjun Zhang wrote:
> > > By the way, you have another similar problem in init_nfp6000_mecsr_sec
> > > with the menum calculation from a bit-field read from an object file.
> > > That also needs to be sanity checked.  Bit-field values of 0 to 3 in the file
> will
> > > result in out of bounds mecfgs array access.
> >
> > I'd checked this part, you mean "menum = _BF (ireg.cpp_offset_lo, 13, 10) -
> 4",
> > right? There's a "-4", so I think it's safe.
> 
> If the bit-field is 0, which it might be with a fuzzed object, then
> menum == (size_t) -4.  That is out of bounds.
> 

I see, thanks, will fix that soon.

> --
> Alan Modra
> Australia Development Lab, IBM