Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(syscall): handle invalid input in bn256 syscalls #1022

Merged
merged 6 commits into from
Mar 22, 2023
Merged

fix(syscall): handle invalid input in bn256 syscalls #1022

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b9fc83a
chore: update deps
magicalne Mar 2, 2023
fa98fbc
fix(syscall): handle errors in bn256
magicalne Mar 13, 2023
9b78758
chore: update syscall simulator
magicalne Mar 13, 2023
26319ed
chore: remove debug log
magicalne Mar 17, 2023
a94c0b7
Fix bn256 test cases
Flouse Mar 22, 2023
c26a37b
Merge pull request #4 from Flouse/fix-bn256-tests
magicalne Mar 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
70 changes: 34 additions & 36 deletions crates/generator/src/syscalls/bn.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,27 +1,36 @@
/// https://github.com/Flouse/bn/tree/0.6.0
use substrate_bn::{
arith::U256, pairing_batch, AffineG1, AffineG2, Fq, Fq2, Fr, Group, Gt, G1, G2,
arith::U256, pairing_batch, AffineG1, AffineG2, FieldError, Fq, Fq2, Fr, Group, GroupError, Gt,
G1, G2,
};

pub struct Error(pub &'static str);
#[derive(thiserror::Error, Debug)]
pub enum BnError {
#[error("Field error: {0:?}")]
G1FieldError(FieldError),
#[error("G1 group error: {0:?}")]
GroupError(GroupError),
#[error("Invalid input length, must be multiple of 192 (3 * (32*2)), actual length: {0}")]
InvalidInputLength(usize),
}

fn read_pt(buf: &[u8]) -> Result<G1, Error> {
let px = Fq::from_slice(&buf[0..32]).map_err(|_| Error("invalid pt"))?;
let py = Fq::from_slice(&buf[32..64]).map_err(|_| Error("invalid pt"))?;
type Result<T> = std::result::Result<T, BnError>;
fn read_pt(buf: &[u8]) -> Result<G1> {
let map_err = BnError::G1FieldError;
let px = Fq::from_slice(&buf[0..32]).map_err(map_err)?;
let py = Fq::from_slice(&buf[32..64]).map_err(map_err)?;
Ok(if px == Fq::zero() && py == Fq::zero() {
G1::zero()
Copy link
Collaborator

@jjyr jjyr Mar 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a breaking change? (is AffineG1(Fq::zero(), Fq::zero()) == G1::zero()? )

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, actually I don't break any change here. But after checking the code, looks like AffineG1(Fq::zero(), Fq::zero()) and G1::zero() are not equivalent.
G1::zero() represents:

        G {
            x: P::Base::zero(),
            y: P::Base::one(),
            z: P::Base::zero(),
        }

And for AffineG1(Fq::zero(), Fq::zero()):

      G {
           x: x,
           y: y,
           z: P::Base::one(),
     };

@Flouse Do you remember about this? Is that intended?

Copy link
Collaborator

@Flouse Flouse Mar 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that is intended.

As my understanding, when px == Fq::zero() && py == Fq::zero(),
it is a special coordinate at infinity with z == P::Base::zero().

similar implementation: https://github.com/openethereum/parity-ethereum/blob/c7f608ec740882eac94038249037ddd955b60d31/ethcore/src/builtin.rs#L408-L414

} else {
AffineG1::new(px, py)
.map_err(|_| Error("invalid pt"))?
.into()
AffineG1::new(px, py).map_err(BnError::GroupError)?.into()
})
}

fn read_fr(buf: &[u8]) -> Result<Fr, Error> {
Fr::from_slice(buf).map_err(|_| Error("invalid fr"))
fn read_fr(buf: &[u8]) -> Result<Fr> {
Fr::from_slice(buf).map_err(BnError::G1FieldError)
}

pub fn add(input: &[u8]) -> Result<[u8; 64], Error> {
pub fn add(input: &[u8]) -> Result<[u8; 64]> {
let mut buffer = [0u8; 128];
if input.len() < 128 {
buffer[0..input.len()].copy_from_slice(input);
Expand All @@ -39,7 +48,7 @@ pub fn add(input: &[u8]) -> Result<[u8; 64], Error> {
Ok(buffer)
}

pub fn mul(input: &[u8]) -> Result<[u8; 64], Error> {
pub fn mul(input: &[u8]) -> Result<[u8; 64]> {
let mut buffer = [0u8; 96];
if input.len() < 96 {
buffer[0..input.len()].copy_from_slice(input);
Expand All @@ -57,54 +66,43 @@ pub fn mul(input: &[u8]) -> Result<[u8; 64], Error> {
Ok(buffer)
}

pub fn pairing(input: &[u8]) -> Result<[u8; 32], Error> {
pub fn pairing(input: &[u8]) -> Result<[u8; 32]> {
if input.len() % 192 != 0 {
return Err(Error(
"Invalid input length, must be multiple of 192 (3 * (32*2))",
));
return Err(BnError::InvalidInputLength(input.len()));
}
let elements = input.len() / 192; // (a, b_a, b_b - each 64-byte affine coordinates)

let ret = if input.is_empty() {
U256::one()
} else {
let mut vals = Vec::with_capacity(elements);
let map_err = BnError::G1FieldError;
for idx in 0..elements {
let a_x = Fq::from_slice(&input[idx * 192..idx * 192 + 32])
.map_err(|_| Error("Invalid a argument x coordinate"))?;
let a_x = Fq::from_slice(&input[idx * 192..idx * 192 + 32]).map_err(map_err)?;

let a_y = Fq::from_slice(&input[idx * 192 + 32..idx * 192 + 64])
.map_err(|_| Error("Invalid a argument y coordinate"))?;
let a_y = Fq::from_slice(&input[idx * 192 + 32..idx * 192 + 64]).map_err(map_err)?;

let b_a_y = Fq::from_slice(&input[idx * 192 + 64..idx * 192 + 96])
.map_err(|_| Error("Invalid b argument imaginary coeff x coordinate"))?;
let b_a_y = Fq::from_slice(&input[idx * 192 + 64..idx * 192 + 96]).map_err(map_err)?;

let b_a_x = Fq::from_slice(&input[idx * 192 + 96..idx * 192 + 128])
.map_err(|_| Error("Invalid b argument imaginary coeff y coordinate"))?;
let b_a_x = Fq::from_slice(&input[idx * 192 + 96..idx * 192 + 128]).map_err(map_err)?;

let b_b_y = Fq::from_slice(&input[idx * 192 + 128..idx * 192 + 160])
.map_err(|_| Error("Invalid b argument real coeff x coordinate"))?;
let b_b_y =
Fq::from_slice(&input[idx * 192 + 128..idx * 192 + 160]).map_err(map_err)?;

let b_b_x = Fq::from_slice(&input[idx * 192 + 160..idx * 192 + 192])
.map_err(|_| Error("Invalid b argument real coeff y coordinate"))?;
let b_b_x =
Fq::from_slice(&input[idx * 192 + 160..idx * 192 + 192]).map_err(map_err)?;

let b_a = Fq2::new(b_a_x, b_a_y);
let b_b = Fq2::new(b_b_x, b_b_y);
let b = if b_a.is_zero() && b_b.is_zero() {
G2::zero()
} else {
G2::from(
AffineG2::new(b_a, b_b)
.map_err(|_| Error("Invalid b argument - not on curve"))?,
)
G2::from(AffineG2::new(b_a, b_b).map_err(BnError::GroupError)?)
};
let a = if a_x.is_zero() && a_y.is_zero() {
G1::zero()
} else {
G1::from(
AffineG1::new(a_x, a_y)
.map_err(|_| Error("Invalid a argument - not on curve"))?,
)
G1::from(AffineG1::new(a_x, a_y).map_err(BnError::GroupError)?)
};
vals.push((a, b));
}
Expand Down
5 changes: 5 additions & 0 deletions crates/generator/src/syscalls/error_codes.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,3 +33,8 @@ pub const GW_ERROR_ACCOUNT_NOT_FOUND: i8 = 85;
pub const GW_SUDT_ERROR_INSUFFICIENT_BALANCE: i8 = 92i8;
pub const GW_SUDT_ERROR_AMOUNT_OVERFLOW: i8 = 93i8;
pub const GW_SUDT_ERROR_UNPERMITTED_ADDRESS: i8 = 94i8;

/* Bn Operations Errors*/
pub const GW_BN_ADD_ERROR: i8 = 100i8;
pub const GW_BN_MUL_ERROR: i8 = 101i8;
pub const GW_BN_PARIING_ERROR: i8 = 102i8;
64 changes: 39 additions & 25 deletions crates/generator/src/syscalls/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
use crate::{
account_lock_manage::AccountLockManage, backend_manage::BlockConsensus, generator::CyclesPool,
syscalls::error_codes::GW_FATAL_UNKNOWN_ARGS,
account_lock_manage::AccountLockManage,
backend_manage::BlockConsensus,
generator::CyclesPool,
syscalls::error_codes::{
GW_BN_ADD_ERROR, GW_BN_MUL_ERROR, GW_BN_PARIING_ERROR, GW_FATAL_UNKNOWN_ARGS,
},
};
use ckb_vm::{
memory::Memory,
Expand Down Expand Up @@ -565,29 +569,35 @@ impl<'a, 'b, S: State + CodeStore + JournalDB, C: ChainView, Mac: SupportMachine
let input_size = machine.registers()[A4].to_u64();
let input = load_bytes(machine, input_addr, input_size as usize)?;

let output = bn::add(&input).map_err(|err| {
let err_msg = format!("syscall SYS_BN_ADD error: {:?}", err.0);
log::error!("{}", err_msg);
VMError::Unexpected(err_msg)
})?;
store_data(machine, output.as_slice())?;

machine.set_register(A0, Mac::REG::from_u8(SUCCESS));
let ret = match bn::add(&input) {
Ok(output) => {
store_data(machine, output.as_slice())?;
Mac::REG::from_u8(SUCCESS)
}
Err(err) => {
log::error!("syscall SYS_BN_ADD error: {:?}", err);
Mac::REG::from_i8(GW_BN_ADD_ERROR)
}
};
machine.set_register(A0, ret);
Ok(true)
}
SYS_BN_MUL => {
let input_addr = machine.registers()[A3].to_u64();
let input_size = machine.registers()[A4].to_u64();
let input = load_bytes(machine, input_addr, input_size as usize)?;

let output = bn::mul(&input).map_err(|err| {
let err_msg = format!("syscall SYS_BN_MUL error: {:?}", err.0);
log::error!("{}", err_msg);
VMError::Unexpected(err_msg)
})?;
store_data(machine, output.as_slice())?;

machine.set_register(A0, Mac::REG::from_u8(SUCCESS));
let ret = match bn::mul(&input) {
Ok(output) => {
store_data(machine, output.as_slice())?;
Mac::REG::from_u8(SUCCESS)
}
Err(err) => {
log::error!("syscall SYS_BN_MUL error: {:?}", err);
Mac::REG::from_i8(GW_BN_MUL_ERROR)
}
};
machine.set_register(A0, ret);
Ok(true)
}
SYS_BN_PAIRING => {
Expand Down Expand Up @@ -620,13 +630,17 @@ impl<'a, 'b, S: State + CodeStore + JournalDB, C: ChainView, Mac: SupportMachine
}
}

let output = bn::pairing(&input).map_err(|err| {
let err_msg = format!("syscall SYS_BN_PAIRING error: {:?}", err.0);
log::error!("{}", err_msg);
VMError::Unexpected(err_msg)
})?;
store_data(machine, output.as_slice())?;
machine.set_register(A0, Mac::REG::from_u8(SUCCESS));
let ret = match bn::pairing(&input) {
Ok(output) => {
store_data(machine, output.as_slice())?;
Mac::REG::from_u8(SUCCESS)
}
Err(err) => {
log::error!("syscall SYS_BN_PAIRING error: {:?}", err);
Mac::REG::from_i8(GW_BN_PARIING_ERROR)
}
};
machine.set_register(A0, ret);
Ok(true)
}
SYS_SNAPSHOT => {
Expand Down
1 change: 1 addition & 0 deletions gwos-evm/polyjuice-tests/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ gw-utils = { path = "../../crates/utils/" }
gw-config = { path = "../../crates/config/", features = ["no-builtin"] }
gw-traits = { path = "../../crates/traits/" }
gw-generator = { path = "../../crates/generator/" }
gw-builtin-binaries = { path = "../../crates/builtin-binaries" }

####
#Sync patch version with godwoken core.
Expand Down
6 changes: 3 additions & 3 deletions gwos-evm/polyjuice-tests/fuzz/gw-syscall-simulator/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -405,7 +405,7 @@ pub unsafe extern "C" fn gw_bn_add(
SUCCESS
}
Err(err) => {
let err_msg = format!("syscall SYS_BN_ADD error: {:?}", err.0);
let err_msg = format!("syscall SYS_BN_ADD error: {:?}", err);
println!("{}", err_msg);
return ERROR;
}
Expand All @@ -427,7 +427,7 @@ pub unsafe extern "C" fn gw_bn_mul(
SUCCESS
}
Err(err) => {
let err_msg = format!("syscall SYS_BN_ADD error: {:?}", err.0);
let err_msg = format!("syscall SYS_BN_ADD error: {:?}", err);
println!("{}", err_msg);
return ERROR;
}
Expand All @@ -449,7 +449,7 @@ pub unsafe extern "C" fn gw_bn_pairing(
SUCCESS
}
Err(err) => {
let err_msg = format!("syscall SYS_BN_ADD error: {:?}", err.0);
let err_msg = format!("syscall SYS_BN_ADD error: {:?}", err);
println!("{}", err_msg);
return ERROR;
}
Expand Down
3 changes: 2 additions & 1 deletion gwos-evm/polyjuice-tests/src/ctx.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ use gw_common::{
CKB_SUDT_SCRIPT_ARGS,
};

use gw_builtin_binaries::file_checksum;
use gw_config::{BackendConfig, BackendForkConfig, BackendType, Resource};
use gw_generator::{
account_lock_manage::{secp256k1::Secp256k1Eth, AccountLockManage},
Expand All @@ -37,7 +38,7 @@ use gw_types::{
prelude::*,
U256,
};
use gw_utils::{checksum::file_checksum, RollupContext};
use gw_utils::RollupContext;

use crate::{
helper::{
Expand Down
3 changes: 2 additions & 1 deletion gwos-evm/polyjuice-tests/src/helper.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ pub use gw_common::{
};
use gw_types::h256::*;

use gw_builtin_binaries::file_checksum;
use gw_config::{BackendConfig, BackendForkConfig, BackendType, ForkConfig, Resource};
pub use gw_generator::{
account_lock_manage::{secp256k1::Secp256k1Eth, AccountLockManage},
Expand All @@ -29,7 +30,7 @@ use gw_types::{
prelude::*,
U256,
};
use gw_utils::{checksum::file_checksum, RollupContext};
use gw_utils::RollupContext;
use rlp::RlpStream;
use std::{convert::TryInto, fs, io::Read, path::PathBuf, sync::Arc};

Expand Down
Loading