[pbs-devel] [PATCH proxmox-backup 0/3] improve status page decoding

[pbs-devel] [PATCH proxmox-backup 0/3] improve status page decoding

[Dominik Csapak]

by better checking the amount of data available vs what we expect,
and by adding some tests (valid and non-valid data)

triggered by a report in the forum:
https://forum.proxmox.com/threads/decode-element-status-failed-failed-to-fill-whole-buffer.93041/

note: 2/3 tests added would fail without the first two patches

Dominik Csapak (3):
  tape: changer: remove unnecesary inquiry parameter
  tape: changer: handle libraries that sends wrong amount of data
  tape: changer: add tests for decode_element_status_page

 src/tape/changer/sg_pt_changer.rs | 162 ++++++++++++++++++++++++++++--
 1 file changed, 153 insertions(+), 9 deletions(-)

-- 
2.30.2

[pbs-devel] [PATCH proxmox-backup 1/3] tape: changer: remove unnecesary inquiry parameter

[Dominik Csapak]

this is never used, so remove it.
Ok, since they are only non public functions.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/tape/changer/sg_pt_changer.rs | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/src/tape/changer/sg_pt_changer.rs b/src/tape/changer/sg_pt_changer.rs
index 3124b96f..2c5434f6 100644
--- a/src/tape/changer/sg_pt_changer.rs
+++ b/src/tape/changer/sg_pt_changer.rs
@@ -24,7 +24,6 @@ use crate::{
     tools::sgutils2::{
         SgRaw,
         SENSE_KEY_NOT_READY,
-        InquiryInfo,
         ScsiError,
         scsi_ascii_to_string,
         scsi_inquiry,
@@ -319,7 +318,6 @@ fn scsi_read_element_status_cdb(
 
 // query a single element type from the changer
 fn get_element<F: AsRawFd>(
-    inquiry: &InquiryInfo,
     sg_raw: &mut SgRaw<F>,
     element_type: ElementType,
     allocation_len: u32,
@@ -342,7 +340,7 @@ fn get_element<F: AsRawFd>(
 
         let data = execute_scsi_command(sg_raw, &cmd, "read element status (B8h)", retry)?;
 
-        let page = decode_element_status_page(&inquiry, &data, start_element_address)?;
+        let page = decode_element_status_page(&data, start_element_address)?;
 
         retry = false; // only retry the first command
 
@@ -394,18 +392,18 @@ pub fn read_element_status<F: AsRawFd>(file: &mut F) -> Result<MtxStatus, Error>
     let mut import_export_slots = Vec::new();
     let mut transports = Vec::new();
 
-    let page = get_element(&inquiry, &mut sg_raw, ElementType::Storage, allocation_len, true)?;
+    let page = get_element(&mut sg_raw, ElementType::Storage, allocation_len, true)?;
     storage_slots.extend(page.storage_slots);
 
-    let page = get_element(&inquiry, &mut sg_raw, ElementType::ImportExport, allocation_len, false)?;
+    let page = get_element(&mut sg_raw, ElementType::ImportExport, allocation_len, false)?;
     import_export_slots.extend(page.import_export_slots);
 
-    let page = get_element(&inquiry, &mut sg_raw, ElementType::DataTransfer, allocation_len, false)?;
+    let page = get_element(&mut sg_raw, ElementType::DataTransfer, allocation_len, false)?;
     drives.extend(page.drives);
 
     // get the serial + vendor + model,
     // some changer require this to be an extra scsi command
-    let page = get_element(&inquiry, &mut sg_raw, ElementType::DataTransferWithDVCID, allocation_len, false)?;
+    let page = get_element(&mut sg_raw, ElementType::DataTransferWithDVCID, allocation_len, false)?;
     // should be in same order and same count, but be on the safe side.
     // there should not be too many drives normally
     for drive in drives.iter_mut() {
@@ -418,7 +416,7 @@ pub fn read_element_status<F: AsRawFd>(file: &mut F) -> Result<MtxStatus, Error>
         }
     }
 
-    let page = get_element(&inquiry, &mut sg_raw, ElementType::MediumTransport, allocation_len, false)?;
+    let page = get_element(&mut sg_raw, ElementType::MediumTransport, allocation_len, false)?;
     transports.extend(page.transports);
 
     let transport_count = setup.transport_element_count as usize;
@@ -668,7 +666,6 @@ fn decode_dvcid_info<R: Read>(reader: &mut R) -> Result<DvcidInfo, Error> {
 }
 
 fn decode_element_status_page(
-    _info: &InquiryInfo,
     data: &[u8],
     start_element_address: u16,
 ) -> Result<DecodedStatusPage, Error> {
-- 
2.30.2

[pbs-devel] applied: [PATCH proxmox-backup 1/3] tape: changer: remove unnecesary inquiry parameter

[Dietmar Maurer]

applied

[pbs-devel] [PATCH proxmox-backup 2/3] tape: changer: handle libraries that sends wrong amount of data

[Dominik Csapak]

if the library sends more data than advertised, simply cut it off,
but if it sends less data, bail out (depending on how much data is
missing, trying to parse it could lead to a panic, so bail out early)

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/tape/changer/sg_pt_changer.rs | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/src/tape/changer/sg_pt_changer.rs b/src/tape/changer/sg_pt_changer.rs
index 2c5434f6..7ff9bc9d 100644
--- a/src/tape/changer/sg_pt_changer.rs
+++ b/src/tape/changer/sg_pt_changer.rs
@@ -692,6 +692,15 @@ fn decode_element_status_page(
             bail!("got wrong first_element_address_reported"); // sanity check
         }
 
+        let len = head.byte_count_of_report_available;
+        let len = ((len[0] as usize) << 16) + ((len[1] as usize) << 8) + (len[2] as usize);
+
+        if len < reader.len() {
+            reader = &reader[..len];
+        } else if len > reader.len() {
+            bail!("wrong amount of data: expected {}, got {}", len, reader.len());
+        }
+
         loop {
             if reader.is_empty() {
                 break;
-- 
2.30.2

[pbs-devel] applied: [PATCH proxmox-backup 2/3] tape: changer: handle libraries that sends wrong amount of data

[Dietmar Maurer]

applied

On 7/28/21 12:05 PM, Dominik Csapak wrote:
> if the library sends more data than advertised, simply cut it off,
> but if it sends less data, bail out (depending on how much data is
> missing, trying to parse it could lead to a panic, so bail out early)
>
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>   src/tape/changer/sg_pt_changer.rs | 9 +++++++++
>   1 file changed, 9 insertions(+)
>
> diff --git a/src/tape/changer/sg_pt_changer.rs b/src/tape/changer/sg_pt_changer.rs
> index 2c5434f6..7ff9bc9d 100644
> --- a/src/tape/changer/sg_pt_changer.rs
> +++ b/src/tape/changer/sg_pt_changer.rs
> @@ -692,6 +692,15 @@ fn decode_element_status_page(
>               bail!("got wrong first_element_address_reported"); // sanity check
>           }
>   
> +        let len = head.byte_count_of_report_available;
> +        let len = ((len[0] as usize) << 16) + ((len[1] as usize) << 8) + (len[2] as usize);
> +
> +        if len < reader.len() {
> +            reader = &reader[..len];
> +        } else if len > reader.len() {
> +            bail!("wrong amount of data: expected {}, got {}", len, reader.len());
> +        }
> +
>           loop {
>               if reader.is_empty() {
>                   break;

[pbs-devel] [PATCH proxmox-backup 3/3] tape: changer: add tests for decode_element_status_page

[Dominik Csapak]

a test for a valid status_page, one with excess data
(in the descriptor as well in the page as a whole)
and a test with too little data

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/tape/changer/sg_pt_changer.rs | 138 ++++++++++++++++++++++++++++++
 1 file changed, 138 insertions(+)

diff --git a/src/tape/changer/sg_pt_changer.rs b/src/tape/changer/sg_pt_changer.rs
index 7ff9bc9d..8451e8e8 100644
--- a/src/tape/changer/sg_pt_changer.rs
+++ b/src/tape/changer/sg_pt_changer.rs
@@ -818,3 +818,141 @@ pub fn open<P: AsRef<Path>>(path: P) -> Result<File, Error> {
 
     Ok(file)
 }
+
+#[cfg(test)]
+mod test {
+    use anyhow::Error;
+    use super::*;
+
+    struct StorageDesc {
+        address: u16,
+        pvoltag: Option<String>,
+    }
+
+    fn build_element_status_page(
+        descriptors: Vec<StorageDesc>,
+        trailing: &[u8],
+        element_type: u8,
+    ) -> Vec<u8> {
+        let descs: Vec<Vec<u8>> = descriptors.iter().map(|desc| {
+            build_storage_descriptor(&desc, trailing)
+        }).collect();
+
+        let (desc_len, address) = if let Some(el) = descs.get(0) {
+            (el.len() as u16, descriptors[0].address)
+        } else {
+            (0u16, 0u16)
+        };
+
+        let descriptor_byte_count = desc_len * descs.len() as u16;
+        let byte_count = 8 + descriptor_byte_count;
+
+        let mut res = Vec::new();
+
+        res.extend_from_slice(&address.to_be_bytes());
+        res.extend_from_slice(&(descs.len() as u16).to_be_bytes());
+        res.push(0);
+        let byte_count = byte_count as u32;
+        res.extend_from_slice(&byte_count.to_be_bytes()[1..]);
+
+        res.push(element_type);
+        res.push(0x80);
+        res.extend_from_slice(&desc_len.to_be_bytes());
+        res.push(0);
+        let descriptor_byte_count = descriptor_byte_count as u32;
+        res.extend_from_slice(&descriptor_byte_count.to_be_bytes()[1..]);
+
+        for desc in descs {
+            res.extend_from_slice(&desc);
+        }
+
+        res.extend_from_slice(trailing);
+
+        res
+    }
+
+    fn build_storage_descriptor(
+        desc: &StorageDesc,
+        trailing: &[u8],
+    ) -> Vec<u8> {
+        let mut res = Vec::new();
+        res.push(((desc.address >> 8) & 0xFF) as u8);
+        res.push((desc.address & 0xFF) as u8);
+        if desc.pvoltag.is_some() {
+            res.push(0x01); // full
+        } else {
+            res.push(0x00); // full
+        }
+
+        res.extend_from_slice(&[0,0,0,0,0,0,0x80]);
+        res.push(((desc.address >> 8) & 0xFF) as u8);
+        res.push((desc.address & 0xFF) as u8);
+
+        if let Some(voltag) = &desc.pvoltag {
+            res.extend_from_slice(voltag.as_bytes());
+            let rem = SCSI_VOLUME_TAG_LEN - voltag.as_bytes().len();
+            if rem > 0 {
+                res.resize(res.len() + rem, 0);
+            }
+        }
+
+        res.extend_from_slice(trailing);
+
+        res
+    }
+
+    #[test]
+    fn status_page_valid() -> Result<(), Error> {
+        let descs = vec![
+            StorageDesc {
+                address: 0,
+                pvoltag: Some("0123456789".to_string()),
+            },
+            StorageDesc {
+                address: 1,
+                pvoltag: Some("1234567890".to_string()),
+            },
+        ];
+        let test_data = build_element_status_page(descs, &[], 0x2);
+        let page = decode_element_status_page(&test_data, 0)?;
+        assert_eq!(page.storage_slots.len(), 2);
+        Ok(())
+    }
+
+    #[test]
+    fn status_page_too_short() -> Result<(), Error> {
+        let descs = vec![
+            StorageDesc {
+                address: 0,
+                pvoltag: Some("0123456789".to_string()),
+            },
+            StorageDesc {
+                address: 1,
+                pvoltag: Some("1234567890".to_string()),
+            },
+        ];
+        let test_data = build_element_status_page(descs, &[], 0x2);
+        let len = test_data.len();
+        let res = decode_element_status_page(&test_data[..(len - 10)], 0);
+        assert!(res.is_err());
+        Ok(())
+    }
+
+    #[test]
+    fn status_page_too_large() -> Result<(), Error> {
+        let descs = vec![
+            StorageDesc {
+                address: 0,
+                pvoltag: Some("0123456789".to_string()),
+            },
+            StorageDesc {
+                address: 1,
+                pvoltag: Some("1234567890".to_string()),
+            },
+        ];
+        let test_data = build_element_status_page(descs, &[0,0,0,0,0], 0x2);
+        let page = decode_element_status_page(&test_data, 0)?;
+        assert_eq!(page.storage_slots.len(), 2);
+        Ok(())
+    }
+}
-- 
2.30.2

[pbs-devel] applied: [PATCH proxmox-backup 3/3] tape: changer: add tests for decode_element_status_page

[Dietmar Maurer]

applied